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

Fix str vs string error introduced by astropy4.3 (ECSV file dtype checking.) #36

Closed
wants to merge 3 commits into from

Conversation

emirkmo
Copy link
Member

@emirkmo emirkmo commented Oct 1, 2021

Astropy 4.3 table module requires string in ecsv headers instead of str for reading from a file.

This PR should be merged before #35 as upgrading to astropy 4.3 will break the lightcurve.py.

Fixes #36 . However, we should ideally fix the entire type checking and update to ECSV V1.0

Astropy 4.3 table module requires string in ecsv headers instead of str for reading from a file.
astropy, scipy, matplotlib, mplcursors, scikit-image
@emirkmo
Copy link
Member Author

emirkmo commented Oct 7, 2021

#27 This handles upgrading astropy to 4.3 after the linked issue is resolved by putting flows on nerd rage. We should also drop support for python 3.6. And unpin as much of our strict package version requirements as possible, unless strictly necessary.

This PR unpins Astropy, but also Matplotlib, scipy, mplcursors, scikit-image.

Tests should tell us if there are any problems with that!

@emirkmo
Copy link
Member Author

emirkmo commented Oct 7, 2021

@rhandberg When nerdrage is setup and you've tested the flows pipeline on it, let's go ahead and start moving to python 3.7+ using this PR as the basis. Will also need to increment version. I also really want to support Astropy >= 4.3 because I need functions in it for my observing scripts.

@rhandberg
Copy link
Contributor

Out of interest, what features in Astropy 4.3 are you using that are not available in the previous versions?

We definitely still want to have version pinned (== instead of >=) since this avoids the code breaking whenever dependencies are updated with backward-incompatible changes. It also ensures reproducibility, since we ensure that two different versions of dependencies does not result in different results from the photometry.

@emirkmo
Copy link
Member Author

emirkmo commented Oct 7, 2021

We definitely still want to have version pinned (== instead of >=) since this avoids the code breaking whenever dependencies are updated with backward-incompatible changes. It also ensures reproducibility, since we ensure that two different versions of dependencies does not result in different results from the photometry.

I disagree, this is bad practice, and makes our work difficult currently. Let me explain:

The issue is currently the flows API and pipeline are in the same package, and hence Pinning the dependency should be done only when it's necessary. The API is essentially a package and needs to be imported into whatever code people are using. Pinned dependencies on such code is bad practice to be avoided. Otherwise, you end up breaking people's python environments. Mine was impossible for example. What we should do is, for each release version, write the package version it was tested against. Perhaps we test against the latest version of the packages available, and pin only if something breaks.

In the scenarios, you use >= for the version of the dependency that the we tested on. It's a hint that if something breaks, you go back to that version. That is standard practice.

For deployed apps or pipelines, we would normally pin versions for reproducibility and to avoid things breaking. However, currently I need to use the flows pipeline API and in the future others will need to use it as well. So we have to relax these pins wherever possible.

The best way out of this would be to make the pipeline and API into two separate packages. Pin dependencies for the pipeline. Leave the API as a library with => hints and release it as an installable package with setup.py.

Out of interest, what features in Astropy 4.3 are you using that are not available in the previous versions?

It's irrelevant because the pins need to be broken for all dependencies but there are several functions within astropy 4.3 that are needed including apply_space_motion for Skycoord package, and to be able to use the latest regions package. Another pin to relax is matplotlib, for plotting improvements for making a dashboard, and so on. It's also good practice to support the latest version of Astropy to take advantage of the ever increasing toolset available in astropy. They're adding some seriously cool stuff each release.

@emirkmo
Copy link
Member Author

emirkmo commented Feb 14, 2022

this is some side effect of updates to ecsv files that happened from Astropy 4.2.3 -> 4.3. See my issue on astropy: astropy/astropy#12840 A strict dtype checking was added that breaks some of our tables.

It's being worked on so we could circumvent this by updating to Astropy 5.1.x when it comes out or sticking to <V4.3, or simply monkey patching ecsv.py.

@emirkmo emirkmo changed the title Fix str vs string error introduced by astropy4.3 Fix str vs string error introduced by astropy4.3 (ECSV file dtype checking.) Feb 14, 2022
@emirkmo
Copy link
Member Author

emirkmo commented Mar 10, 2022

This is now fixed in the backend API and with Astropy V 5.0.2+

@emirkmo emirkmo closed this Mar 10, 2022
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.

2 participants