search

GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
224 stars 106 forks source link

Add PhotonArray.fromArrays constructor #1109

Closed jmeyers314 closed 3 years ago

jmeyers314 commented 3 years ago

Addresses #1108 . This version never copies an input array, but instead raises an error if a copy would be required. There is both a sanity-checked "public" ctor PhotonArray.fromArrays, and a "private" unchecked ctor PhotonArray._fromArrays.

jmeyers314 commented 3 years ago

I'm sad that GHA cancels all the builds as soon as any fails.

Looks like warnings are only caught by assert_warns in py27 if they are the first occurrence of that warning maybe? Should we just have assert_warns always succeed in py27?

(And separately, I think we should start thinking about dropping support for py27...)

rmjarvis commented 3 years ago

I'm sad that GHA cancels all the builds as soon as any fails.

I think the logic is that if something fails, then you'll need to rerun everything anyway, so they're saving cpu cycles. But I agree, it was nice to be able to see the full set of failures after each push.

Looks like warnings are only caught by assert_warns in py27 if they are the first occurrence of that warning maybe? Should we just have assert_warns always succeed in py27?

I thought this was always the default behavior. Once a warning is emitted, I thought python turned off subsequent warnings from that same bit of code. Otherwise, if you hit a warning in a loop for instance, you can end up with a huge cascade of warnings that are all the same thing. Our assert_warns context both checks that a warning is emitted and resets the count so that the next time, it will also be emitted.

The problem is that your new test is emitting that warning, which makes python turn off that warning for the later test that checks for it.

I think the solution is either to fix the test so that it doesn't warn, or document that it is an expected warning by wrapping the line that does so in an assert_warns context.

rmjarvis commented 3 years ago

(And separately, I think we should start thinking about dropping support for py27...)

I think I probably agree. Maybe on our next release, declare that this will be the last one to support py2.7 and get rid of it at that point on main.

jmeyers314 commented 3 years ago

I thought this was always the default behavior.

I guess it could be. I only saw failures on the py27 branches, and didn't see anything locally. But maybe there's something about nose tests in py27 that changes the order the tests are run in.

Our assert_warns context both checks that a warning is emitted and resets the count so that the next time, it will also be emitted.

Ah, I didn't realize it reset the counter. I wrapped the new code in an assert_warns. Looks like CI is passing now.