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

Merge develop_aircraft into develop #253

Merged
merged 77 commits into from
Mar 25, 2024

Conversation

colin-harkins
Copy link
Collaborator

This pull request merges the develop_aircraft branch into the develop branch. Nothing here has changed from #246.

colin-harkins and others added 30 commits August 23, 2022 14:01
Add basic pairing for aircraft observations
Merge all new features from develop into develop_aircraft
Add ability to read aircraft obs from .csv file
Update resample_stratify function in tools.py to work with new stratify version
… (i.e., if altitude variable is present), no changes in surfaceplots
…rquartile_style: 'shading' (interquartile range only) or 'box' (box as interquartile range, whisker low and high refer to 10th-90th percentile)
…ow in aircraftplots.py, and removed plotting from driver.py (it just calls this function from airplots)
…r timeseries with altitude profile plots, similar to the optional y-xis limit option as demonstrated and added for vertprfile plots
…ase, with edited yaml file path i.e., in ../examples/yaml/...
@colin-harkins
Copy link
Collaborator Author

Looks like I need to fix something to get the tests to pass. I'll update today.

@colin-harkins
Copy link
Collaborator Author

I couldn't quite figure out how to get the docstring for the loop_pairing function to work when providing examples of the inputs, so i just gave locations where examples can be found for now. Unless you have any recommendations right now, I can try to figure out a way to do this in the future once I can get more familiar with the syntax of sphinx docstrings.

@rschwant
Copy link
Collaborator

@colin-harkins updating to showing an example in jupyter notebook works for the loop_pairing function. Is this ready then? I will look through it early this week if so, and @quaz115 if you can double check it all looks good too that would be great. We need 2 reviewers to go into develop.

@quaz115
Copy link
Collaborator

quaz115 commented Mar 20, 2024

I couldn't quite figure out how to get the docstring for the loop_pairing function to work when providing examples of the inputs, so i just gave locations where examples can be found for now. Unless you have any recommendations right now, I can try to figure out a way to do this in the future once I can get more familiar with the syntax of sphinx docstrings.

@colin-harkins @rschwant Regarding the docstring I think the line at which sphinx-build error happens is causing the issue: i.e.

An example can be found in examples/jupyter_notebooks/Monet-example_aircraft_pairing_loop.

You can try with closing the file path with backticks like this ?

An example can be found in 'examples/jupyter_notebooks/Monet-example_aircraft_pairing_loop.ipynb'

Hoping the above fix might be sufficient, if not also look at the following suggestion (since Sphinx-build failed at line right before where file paths have asterisks as well):

Sphinx seems to cause issue with CI/CD tests while reading file paths especially when asterisks (*) in the file paths or in text. Check for other instances as well where there is need to enclose the file paths in double backticks (which is used for inline code in reStructuredText e.g., docstring with file paths ).

For example: you can also try changing current format (as a generic example):

file_pairs_yaml = 'dirpath/file*.yaml'

TO

Either:

file_pairs_yaml = 'dirpath/file\*.yaml' # If you are using asterisks literally. (but that is not the case as they are used as wildcards for reading multiple files)

OR a better alternative would be to simply highlight the text without any misunderstanding by Sphinx:

Example:

'file_pairs_yaml = dirpath/file*.yaml' # Using backticks to enclose paths or commands.

@rschwant
Copy link
Collaborator

@colin-harkins Would it be better to merge your commits from colin-harkins:develop_aircraft to NOAA-CSL:develop_aircraft and then do a merge from NOAA-CSL:develop_aircraft to NOAA-CSL:develop, or does this not matter? I want to close out the develop_aircraft branch after this merge.

@colin-harkins
Copy link
Collaborator Author

@rschwant I don't think it really matters since we want to close out the develop_aircraft branch after this. That's what I was thinking when submitting this pull request anyway.

Copy link
Collaborator

@rschwant rschwant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, we can keep it coming from your fork then, since I double checked your fork is updated with the latest develop_aircraft branch and we are deleting this branch after the merge anyway. Everything looks good to me. Quazi will be the second approver and he may not get to this until next Monday.

@colin-harkins
Copy link
Collaborator Author

@quaz115 I fixed the issue with the docstring by using a literal block and then also enclosed the file paths in double backticks and everything seems to work. Thanks for the help with that.

@quaz115
Copy link
Collaborator

quaz115 commented Mar 21, 2024

@quaz115 I fixed the issue with the docstring by using a literal block and then also enclosed the file paths in double backticks and everything seems to work. Thanks for the help with that.

@colin-harkins Awesome! Glad it worked. I will try to review the PR by today or tomorrow and then use this to submit a new PR with the remaining aircraft vs model plots (curtain and spatial)

Copy link
Collaborator

@quaz115 quaz115 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed both the aircraft plotting and netcdf pairing updates and tested as well, they work properly and have the most recent code changes.

@colin-harkins colin-harkins merged commit 22502cf into NOAA-CSL:develop Mar 25, 2024
4 checks passed
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.

None yet

3 participants