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

Release 0.9.2 #392

Merged
merged 4 commits into from
Feb 11, 2021
Merged

Release 0.9.2 #392

merged 4 commits into from
Feb 11, 2021

Conversation

mperrin
Copy link
Collaborator

@mperrin mperrin commented Feb 9, 2021

As discussed in slack, let's have a v0.9.2 release:

for poppy + webbpsf releases here is my thinking currently. It’s turned into a bit of a tangle given the interlinked dependencies between these two (and the optics lab code on the research side).
I’m going to make an v0.9.2 release branch of poppy containing the current code - but backing out the change in _get_optical_system syntax that I had meant to be backwards compatible but turned out not to be. I will make sure that both the released webbpsf 0.9.1 and current develop run correctly on that brach of poppy. That can be a poppy release v0.9.2 but does not need a webbpsf release, I don’t think.
That will give a stable point right now, so the optics lab code can continue with that for the time being, and then on top of that I can then finish up the 1.0 release and sign conventions change without disrupting anything in the lab
(which eventually wants that update, but I have enough moving pieces right now I want to treat it as a separable problem rather than having to deal with everything all at once)

Toward that end I:

  • Copied over and updated the release notes that had been originally drafted for v1.0
  • Reverted the back-incompatible change in the get_optical_system syntax
  • Tested that both the released webbpsf v0.9.1 and current develop webbpsf pass their own tests on this branch. That's true and they do.

@mperrin mperrin self-assigned this Feb 9, 2021
@mperrin mperrin requested a review from shanosborne February 9, 2021 21:05
@codecov
Copy link

codecov bot commented Feb 9, 2021

Codecov Report

Merging #392 (1716c2a) into develop (3f3fd28) will increase coverage by 0.05%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #392      +/-   ##
===========================================
+ Coverage    74.16%   74.21%   +0.05%     
===========================================
  Files           17       17              
  Lines         5829     5829              
===========================================
+ Hits          4323     4326       +3     
+ Misses        1506     1503       -3     
Impacted Files Coverage Δ
poppy/instrument.py 57.07% <50.00%> (+0.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f3fd28...1716c2a. Read the comment docs.

Copy link
Contributor

@shanosborne shanosborne left a comment

Choose a reason for hiding this comment

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

I've confirmed that this version of poppy works fine with the stable branch of webbpsf, but I'm seeing the tests on the develop branch of webbpsf are failing with this branch.



**Software Infrastructure Updates and Internals:**
* The minimum numpy version is now 1.16. (:pr:`356` by :user:`mperrin`)
Copy link
Contributor

Choose a reason for hiding this comment

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

PR 356 was for changing the minimum python version to 3.6 (i think the numpy comment if left over from a copy/paste)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha, looks like this was actually your PR #367 which updated the numpy version. But in fact it looks like that only updated the CI test settings in tox.ini to have 1.16, and we still have 1.13 in the setup.py file. Seems like we should make that consistent.

In fact it seems like we have three sets of version information right now? Getting complicated. THere's

  • requirements.txt, for creating environments with up-to-date packages, and use by dependabot for testing new versions
  • setup.py for setting minimum versions and allowing flexibility to people installing this in different environments
  • tox.ini for setting which versions get used on the CI system, including multiple options.

I don't see any easy way to simplify that but it's not exactly simple. Can you do a quick check through those for any other inconsistencies, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha yes you're right. I've just gone through the files and updated an inconsistencies with a new commit in #395

@mperrin
Copy link
Collaborator Author

mperrin commented Feb 10, 2021

I'm seeing the tests on the develop branch of webbpsf are failing with this branch.

Can you post a copy of the test log with the failures here? For me this is working correctly. It's possible you may be seeing failures for an unrelated reason having to do with expected changes in data files.

@shanosborne
Copy link
Contributor

webbpsf-test-output.txt
Here's my terminal output, I confirmed I'm on the develop branch of webbpsf (and it's up to date) and on this branch of poppy, and I'm getting AttributeError: 'NIRCam' object has no attribute '_extra_keywords' on the tests

@mperrin
Copy link
Collaborator Author

mperrin commented Feb 10, 2021

I'm baffled by that terminal output; those look like the right git commit IDs, and match what I have here locally myself, but for me this is all passing.

I reverted the change in the get_optical_system syntax in this branch; so when run with webbpsf develop this should be using a call stack like this :

FGS.calc_psf = webbpsf.JWInstrument.calc_psf()
-> calls webbpsf.SpaceTelescopeInstrument.calc_psf(), which is the inherited poppy.Instrument.calc_psf()
     -> calls poppy.Instrument._get_optical_system()
     -> calls webbpsf.SpaceTelescopeInstrument.get_optical_system()
          # and that defines the _extra_keywords among its many other steps

I have verified the above is what happens when I run this.

The problem occurs when instead the following happens on poppy v0.9.1 and webbpsf develop:

FGS.calc_psf = webbpsf.JWInstrument.calc_psf()
-> calls webbpsf.SpaceTelescopeInstrument.calc_psf(), which is inherited poppy.Instrument.calc_psf()
     -> calls poppy.Instrument._get_optical_system(), which just runs directly and doesn't call the subclassed version.

The only way I can recreate the behavior you are seeing is if I install poppy 0.9.1 and run it with webbpsf develop...

@mperrin mperrin mentioned this pull request Feb 10, 2021
@mperrin
Copy link
Collaborator Author

mperrin commented Feb 10, 2021

@Skyhawk172 has also tested this branch with webbpsf develop and has the same result as me (it all works). Meanwhile @shanosborne has retested and still has the same result she did before (it doesn't work). This is curious, to say the least. @shanosborne will continue investigation tomorrow.

Copy link
Contributor

@shanosborne shanosborne left a comment

Choose a reason for hiding this comment

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

Upon further investigation, the problem was conda related and not poppy related. So this PR is good to go!

@mperrin mperrin merged commit bf0531d into spacetelescope:develop Feb 11, 2021
@mperrin mperrin deleted the release_0.9.2 branch February 11, 2021 21:27
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