AstarVienna / skycalc_ipy

iPython version of skycalc_cli
GNU General Public License v3.0
4 stars 7 forks source link

Attempt to fix unit kerfuffle #28

Closed teutoburg closed 1 year ago

teutoburg commented 1 year ago

Apparently, ESO changed the wavelength unit from µm to nm in the switch to the new server, but also now include that information in the header. This PR should fix that generally, by using the units from the header instead of hardcoding µm, but retain it as a fallback if no unit information is found.

Also, the unit for the flux is not very well converted by astropy.table.Table, although creating it directly from the header (i.e. u.Unit(skm.data[1].header["TUNIT2"])) seems to work fine. I added a simple check to at least see if there's nothing unexpected in the flux unit before overwriting it...

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0d1eece) 99.02% compared to head (5c565da) 99.02%. Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #28 +/- ## ======================================= Coverage 99.02% 99.02% ======================================= Files 2 2 Lines 205 205 ======================================= Hits 203 203 Misses 2 2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hugobuddel commented 1 year ago

Easiest way to update the files in skycalc_ipy/data is to simply remove them, run pytest and then commit the two changed files back

teutoburg commented 1 year ago

Running pytest doesn't download them to that data dir, but to the location where the pip installed package is located. And it's 16 files rather than two. I deleted the old ones anyway, apparently they weren't used by the tests anyway?

teutoburg commented 1 year ago

Perhaps that entire directory can be dropped though, because we can use ScopeSim_data for that.

I want to change the cache location (and download handling generally) to something better, but that'll be in another PR, to keep it simple and separate 🙂

hugobuddel commented 1 year ago

Running pytest doesn't download them to that data dir, but to the location where the pip installed package is located.

Yeah, it downloads the files to that directory if you install skycalc_ipy with pip install -e .; that was the idea behind it. Downloading to the site-packages directory is a bad thing.

And it's 16 files rather than two. I deleted the old ones anyway, apparently they weren't used by the tests anyway?

I left those two files in when I moved the rest to ScopeSim_data. I think my reasoning was that ScopeSim_data is not yet ready to be used by end-users, and having this directory there allowed people to still make use of the caching functionality. Those two files were just an example. But it is better to delete them.

I want to change the cache location (and download handling generally) to something better, but that'll be in another PR, to keep it simple and separate 🙂

It is already possible to specify the download directory. ScopeSim_data does that.

Using this directory as default made (some) sense when the idea was still to include the data in the package. But that is now moved to ScopeSim_data.

Let's merge this and do the rest later.