-
Notifications
You must be signed in to change notification settings - Fork 1
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
Rcv0 2 0 #30
Rcv0 2 0 #30
Conversation
Update develop with all RC0.1.3 changes
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.
Some updates to the core documents. We should probably finalize this soon, as dependabot is getting cranky about the numpy version cap in main.
CHANGELOG.md
Outdated
@@ -2,6 +2,31 @@ | |||
All notable changes to this project will be documented in this file. | |||
This project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|||
## [0.2.0] - 2022-06-10 |
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.
make sure to update
Co-authored-by: Jeff Klenzing <19592220+jklenzing@users.noreply.github.com>
hmm........ we broke something
|
Tests are passing with latest pysat. |
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 know I already approved, but this would reduce the meta deprecation warnings throughout.
Co-authored-by: Jeff Klenzing <19592220+jklenzing@users.noreply.github.com>
README.md
Outdated
The caps on numpy and python stem from compatibility with the maximum | ||
supported pandas version. Version 0.2.0 will rewrite the routines to remove | ||
`Panel`. | ||
and for the Space Physics community. | ||
|
||
| Common modules | Community modules | | ||
| -------------- | ----------------- | | ||
| matplotlib | pysat | |
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.
Should pysat be version limited?
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.
Yes, after some digging I found the we need Constellation instantiation support given a list of instruments, which was added to pysat in v3.0.1. Tests were passing here with that pysat version. Added the req to requirements.txt and the readme.
Co-authored-by: Angeline Burrell <aburrell@users.noreply.github.com>
README.md
Outdated
| numpy<1.20 | | | ||
| pandas<0.24 | | | ||
|
||
| matplotlib | pysat >= 3.0.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.
| matplotlib | pysat >= 3.0.1 | | |
| matplotlib | pysat >= 3.0.2 | |
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.
Seems like the safest thing? If not needed, that's fine.
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.
Needed for the unit tests since we added the use_header
kwarg to those. I don't think it's needed in the core code.
requirements.txt
Outdated
pysat>=3.0.1 | ||
xarray<2022.06.0 |
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.
If going this direction, update the xarray limit in the README
setup.cfg
Outdated
numpy | ||
pandas | ||
pysat>=3.0.2 | ||
xarray<2022.06.0 |
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.
There shouldn't be any version caps here
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 version limits aren't applied, or checked, during installation without specifying them here
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.
From docs on install_requires, https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/,
Additionally, it’s best practice to indicate any known lower or upper bounds.
The root version requirement on xarray should be placed on pysat, and re-released, so that the environments here work more out of the box.
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.
From your link:
Lastly, it’s important to understand that install_requires is a listing of “Abstract” requirements, i.e just names and version restrictions that don’t determine where the dependencies will be fulfilled from (i.e. from what index or source). The where (i.e. how they are to be made “Concrete”) is to be determined at install time using pip options. 1
Requirements files
Requirements Files described most simply, are just a list of pip install arguments placed into a file.Whereas install_requires defines the dependencies for a single project, Requirements Files are often used to define the requirements for a
While the limits don't appear to be breaking the setup the way that they used to, it still seems cleaner to use the requirements file for this. Might be best to discuss our standards at the next meeting.
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.
From your link:
Lastly, it’s important to understand that install_requires is a listing of “Abstract” requirements, i.e just names and version restrictions
While the limits don't appear to be breaking the setup the way that they used to, it still seems cleaner to use the requirements file for this. Might be best to discuss our standards at the next meeting.
I'll put it on the agenda for Wed. though it is possible I may not be available. Government depending.
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 checked the linked discussion from pysatSpaceWeather: pypa/setuptools#1951
This particular comment notes the anti-pattern is loading requirements.txt into install_requires. Further notes that the biggest issue with loading requirements.txt is the potential for pinned versions set with a ==
. Notes a compromise would be to allow requirements.txt to be loaded into install_requires as long as pinned versions aren't used.
pypa/setuptools#1951 (comment)
The python docs https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/ say
install_requires is a [setuptools](https://packaging.python.org/en/latest/key_projects/#setuptools) setup.py keyword that should be used to specify what a project minimally needs to run correctly. When the project is installed by [pip](https://packaging.python.org/en/latest/key_projects/#pip), this is the specification that is used to install its dependencies.
I checked to see what was above this doc for greater context to see if this install_requires discussion was limited to a particular sub-situation but didn't see anything.
From what I've read setting minimal version requirements in install_requires in setup.cfg for pip installs is the correct thing to do. pip uses the information from setup to determine dependencies. For user pip installs of pysatPenumbra packages to work reliably we should specify a minimum pysat version in setup, 3.0.4, when that version is released. A similar condition would also be in requirements.txt, but strictly speaking the requirements and setup restrictions are there for different reasons thus may not always be the same. requirements.txt is not loaded into install_requires.
I'm not opposed to using a .toml file but I don't understand why that is better.
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 think it comes down to the idea that the format of a cfg is loosely defined. See https://peps.python.org/pep-0518/#sticking-with-setup-cfg for why toml was recommended above cfg
check if matplotlib dependence is real |
Tests stopped working due to some kind of numpy and xarray issue. Not knowing what is going on, I bumped the numpy version up to 1.21.... How is it that working software always breaks just before release? 🤷 |
Now a lone windows test failure from matplotlib with the suggestion that perhaps Tcl wasn't installed properly. |
Description
Addresses #3, #15, #28, #29
A large range of changes to get pysatSeasons generally up to the modern era.
collections.deque
usecomputational_form
->to_xarray_dataset
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. Please also list any relevant details for
your test configuration
Test Configuration:
Checklist:
CHANGELOG.md
, summarizing the changes