Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

7 lose fork from read icesat 2 and copy add to its own branch #8

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mochell
Copy link
Collaborator

@mochell mochell commented Sep 1, 2023

added read-ICESat-2 file to our git. functioning was testes see #5

@mochell mochell self-assigned this Sep 1, 2023
@mochell mochell linked an issue Sep 1, 2023 that may be closed by this pull request
@mochell mochell assigned cpaniaguam and unassigned mochell Sep 1, 2023
@cpaniaguam
Copy link
Member

@mochell are these the files from the read-ICESat-2 repo you are modifying to suit your particular needs for this project? If so I think it'd be better to have this in a different repo. This code is already packaged and can easily be imported into the other project as a dependency using pip or something similar.

@mochell
Copy link
Collaborator Author

mochell commented Sep 5, 2023

We could do that but its literaly 1 line in 1 file different. All these files are just needed to run the test. We will not need this code any more once we switched to sliderule.

@cpaniaguam
Copy link
Member

@mochell Is there a way to know the specific line and file that was modified? If the repo is forked from the original one, changes to those files can be tracked, and a baseline for that code can be established. Also, if many of the tests are not needed, there is really no good reason to keep them in the repo. We could instead create tests for the new feature that you are adding.

@mochell
Copy link
Collaborator Author

mochell commented Sep 5, 2023

Id rather spend not much time on this old way to retrieve the data, read-ICESat2 is not a well maintained code base and nearly all features from there are implimented in SlideRule (its the same coder). So this little dirty copying of all files in the repro should do the job to run the test.

the only change I made compared to the git repository you can see by comparing
modules/read-ICESat-2/scripts/nsidc_icesat2_associated2.py
vs
modules/read-ICESat-2/scripts/nsidc_icesat2_associated.py

@cpaniaguam
Copy link
Member

Id rather spend not much time on this old way to retrieve the data

@mochell I understand your concern about spending extra time on a feature that will soon be superseded. However, we'd need to do this later anyway (if PR were to be merged) and it's better to clean up things when they are fresh in our minds! This is the kind of thing we would take care of on our end so you can focus on the big picture.

For now I propose we use the code in this branch for testing purposes and not merge it to master to avoid the clean up later. Thoughts?

@mochell
Copy link
Collaborator Author

mochell commented Sep 6, 2023

Yes I think just keeping it in the branch for now Is also a good solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lose fork from read-ICESat-2 and copy add to its own branch
2 participants