-
Notifications
You must be signed in to change notification settings - Fork 9
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
Sp3 changes + unittesting #32
Conversation
…ransform.py + Testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big contribution and cleans up a lot of the code, thanks a lot for this!
The testing you've introduced is fantastic and I think it's definitely something all of us will need to continue with future PRs.
tests/test_sp3.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't run a test of this myself, but looks good from glancing through it.
Would be good to get @EugeneDu-GA to look at his sp3 files and see if there are any other corner cases that we should add here
gnssanalysis/gn_io/sp3.py
Outdated
temp_sp3.set_index(temp_sp3.PV_FLAG.astype(str), append=True, inplace=True) | ||
return temp_sp3 | ||
|
||
|
||
def read_sp3(sp3_path, pOnly=True, nodata_to_nan=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output type-hint is missing here.
Also, I think @EugeneDu-GA should test any sp3 files he was having trouble with against this function
@@ -35,6 +32,31 @@ | |||
# File descriptor and clock | |||
_RE_SP3_HEAD_FDESCR = _re.compile(rb"\%c[ ]+(\w{1})[ ]+cc[ ](\w{3})") | |||
|
|||
|
|||
_SP3_DEF_PV_WIDTH = [1, 3, 14, 14, 14, 14, 1, 2, 1, 2, 1, 2, 1, 3, 1, 1, 1, 1, 1, 1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff. Thanks for this, I'd been meaning to make the use of FWF more robust.
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install boto3 click hatanaka matplotlib numpy pandas plotext==4.2 plotly pymongo pytest scipy tqdm unlzw3 typing_extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This duplicates specification of our requirements. It's not as simple as just pointing at requirements.txt
though, as there isn't one.
Requirements are specified in setup.py
, though that could be modified to read them from a requirements.txt
file for consistency... I prototyped that idea and it works, but it's hacky because I have to handle possible commenting. Time for me to read about the differences between setup.py and requirements.txt and how to do this properly...
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install -r requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at it a bit more I think there's a better way than my previous suggestion.
I'm dropping the suggestion in line here, based on my comment on the specific commit
pip install -r requirements.txt | |
sed -i "/{{VERSION_PLACEHOLDER}}/d" setup.py | |
pip install . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the versioner in place, if there is a tag, the version is the tag, else the commit hash.
We change it in the setup.py so we can install manually some to make some tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, this is in the housekeeping branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit 9e96872d
tested working. Looks good for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With testing complete by @EugeneDu-GA , I'm happy for this to go in as well 👍
f8e6493 tested working for sp3 files with velocities, with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print statement removed - happy for this to go in now 👍
Set of changes in the read SP3 triggered by trying to read a corrupted sp3 file. now each epoch is read separately to be able to read most of a corrupted file.
conversion / add of some of the preexisting test into the python unittest package framework & creation of a pipeline to run the test at the push on the repo.
More to come.