-
Notifications
You must be signed in to change notification settings - Fork 37
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
Adding SURFRAD Capabilities #689
Conversation
AdamTheisen
commented
Jun 19, 2023
•
edited
Loading
edited
- Tests added
- Documentation reflects changes
- PEP8 Standards or use of linter
- Xarray Dataset or DataArray variable naming follows 'ds' or 'da' naming
…and made some updates to corresponding functions
Looks like there are some errors with the PySP2 tests |
@AdamTheisen it appears that there is some 0 shaped array being multiplied within the try/except block... if np.isfinite(base):
try:
height = data_fit.max() - base
pos = np.argmax(data)
except ValueError:
height = np.nan
pos = np.nan
try:
start = np.where((data - base) < 50)[0]
> if start == []:
E ValueError: operands could not be broadcast together with shapes (14,) (0,) |
Nothing changed within the pysp2 repo itself, hmm so curious why it's failing now |
pysp2 does not currently have a nightly build running/testing with dependencies, so I suspect is has to do with some upstream library cutting a new release |
@zssherman I suspect numpy is the culprit here... https://github.com/numpy/numpy/releases/tag/v1.25.0 They cut a new release two days ago, and there are mentions about deprecated masking functionality/changes with array shapes. |
@mgrover1 @AdamTheisen Looks like we need a change upstream. So when it checks the start for empty list, check if its a numpy array
|
Upstream as in pysp2? |
@mgrover1 I believe so |
Ahh the good old |
Found another issue with the plot and the text showing up as expected while looking so it was a good error to help improve the example! There was also a deprecation warning with the fix I had put in so had to redo that. |
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.
@AdamTheisen - does their documentation include anything about the standard_name
for the variables? I think that would be helpful include here if that is the case.
The only documentation they really have is what's in the README file. They don't have anything about standard_names from what I can see. |
Gotcha - any chance you could take a look at this table And see which ones would map? One benefit of using a |
Looks good to me, tests are now passing! |
Just as a note, I did not get a chance to add in standard names yet. Was hoping to look into that tomorrow |
Sounds good - I think that makes sense for separate PR 😄 |
* ENH: Adding two new examples for heatmap and size distribution plots and made some updates to corresponding functions * ENH: Copy and paste error. Updated the title and descriptions * ENH: Adding SURFRAD capabilities * ENH: Bug fix for discovery test * ENH: Updating for bug in testing * ENH: Bug fixes and improvements to bottom margin