Conversation
jmeyers314
left a comment
jmeyers314
left a comment
There was a problem hiding this comment.
Looks good to me Mike. Just a small handful of comments/questions.
| adding sub-classes, which may provide more specific information about the | ||
| nature of an error. So far, sub-classes include GalSimHSMError for errors | ||
| during HSM measurements and GalSimRangeError for attempted use of input | ||
| parameters outside of allowed ranges. (#755) |
There was a problem hiding this comment.
It looks like you defined a bunch more errors than just GalSimHSMError and GalSimRangeError. I don't think they all need to be listed in the changelog, but maybe on a wiki page (cited here)?
There was a problem hiding this comment.
Thanks. I guess I wrote this long before I had finished adding more error types. I think maybe I'll just reference errors.py for the list of error types. For most people, the important thing is that they can catch GalSimError if they want to distinguish errors in GalSim from other kind of errors.
| # point to the same object. So we'll just check for that for _jac and _offset. | ||
| for attr in ['_jac', '_offset']: | ||
| # point to the same object. So we'll just check for that for _jac, _offset, _flux_ratio. | ||
| for attr in ['_jac', '_offset', '_flux_ratio']: |
There was a problem hiding this comment.
Switch this list to tuple too?
Actually, grepping for " in \[" yields 155 results; should we go through and switch them all (or at least the ones that lists of hard-coded strings)?
There was a problem hiding this comment.
OK. Probably pedantic optimization in most cases, but I did read that for things like this tuples are more efficient than lists...
|
|
||
| if 'image_pos' in config and 'world_pos' in config: | ||
| raise AttributeError("Both image_pos and world_pos specified for Scattered image.") | ||
| raise galsim.GalSimConfigError( |
There was a problem hiding this comment.
This feels more like a GalSimIncompatibleValuesError to me.
Is the goal to only raise either GalSimConfigError or GalSimConfigValueError during config processing?
There was a problem hiding this comment.
I could at least go to GalSimConfigValueError I guess. But yes, I wanted GalSimConfigError to be a base class for all errors related to parsing a config dict. That's a kind of user error that someone might want to guard against in their code, since it's possible that the coder doesn't have control over what config dict will be parsed.
| file_name = os.path.join(meta_data.share_dir,'acs_I_unrot_sci_20_cf.fits') | ||
| if not os.path.isfile(file_name): | ||
| raise IOError("The file '"+str(file_name)+"' does not exist.") | ||
| raise OSError("The file %r does not exist."%(file_name)) |
There was a problem hiding this comment.
Let the fits.read operation below raise this error?
There was a problem hiding this comment.
I think I'd rather keep it separate. The file not existing is a straightforward error that the user can probably deal with. The next line should be an unusual occurrence that probably indicates some kind of corruption of the file. I did add the text of the original error though, in case that's helpful for the user to figure out what was wrong.
| self._sbp.shoot(photons._pa, ud._rng) | ||
| except RuntimeError: | ||
| xi = self.x_interpolant.__class__.__name__ | ||
| raise GalSimError("Photon shooting is not practical with x_interpolant of type %s"%xi) |
There was a problem hiding this comment.
Is this the only reason why _sbp.shoot might raise a RuntimeError?
Looking through SBInterpolatedImageImpl::shoot, I see a couple of asserts related to photons.size() for example (though I wouldn't know how to actually trigger them). (Also, I suppose there's no need for both asserts there and the if (N <=0 || ...) test).
There was a problem hiding this comment.
Thanks. This should be a with convert_cpp_errors context now. I think I wrote this before I made that context class.
| u, v = np.meshgrid(u, u) | ||
| self._rho = u + 1j * v | ||
| return self._rho | ||
| f1 = np.fft.fftfreq(self.npix, 1./self.pupil_plane_size) |
There was a problem hiding this comment.
I don't think we need f1 and f2 here. Actually, since we switched the Zernike class to take x and y instead of rho and rhosqr, I think we may be able to get rid of this property and rsqr entirely.
There was a problem hiding this comment.
That's up to you I guess. There are a few functions (these plus some others) that aren't used anywhere, but I added simple unit tests for them, since I thought maybe they were still useful for debugging purposes. But if you think you won't need them anymore, feel free to zot them. (I removed the unused f1, f2 calculations at least.)
| raise TypeError('Method called on dead object') | ||
| return self.f(self.c(), *args) | ||
|
|
||
| def EnsureDir(target): |
There was a problem hiding this comment.
Is there a reason this is CamelCase?
There was a problem hiding this comment.
Not particularly. I just moved it wholesale from where it was in config/output.py since I thought it was more generally useful. The style there is mostly CamelCase.
|
Thanks Josh. I think I addressed all your suggestions. Let's plan to merge next Wednesday unless anyone needs more time. I'm giving a week because I know @esheldon is at a DES meeting this week and I'm hoping he might have a chance to look at this a bit, which he probably won't have time to do this week. |
|
sorry, I was on vacation. I didn't get a chance to test this but I will before an official release. |
|
Great. Please do send any comments along if you notice anything that ought to be changed. Thanks! |
3 participants
This is the last set of changes that I would like to have included in version 2.0.
@esheldon requested on Issue #755 that we emit some kind of GalSim-specific errors rather than generic RuntimeError (especially) or ValueError. This is kind of an API change, so I think it makes sense to include it in GalSim 2.0 where API changes are acceptable. (Not that it probably breaks anyone's code, but technically some catch statements that would have working in 1.6 would fail now.)
The main thing for reviewers to look at is errors.py where I define all the new exception classes. I also give guidelines for when it is appropriate to use non-GalSim errors. My rule of thumb for this is that if Python were more strongly typed, then things like TypeError, AttributeError, NotImplementedError would be places where the compiler would have noticed the error and failed. I.e. the error is very clearly some kind of coding error, not an error of user input or some run-time exceptional occurrence. (I think this is in the spirit of the suggestions Erin made in the thread of #755, which is worth reviewing.)
Everything else is now a GalSimError or (usually) one of its sub-classes. The base GalSimError is kind of a catch-all that acts like a RuntimeError. There are (many) others that try to give more specific information about the exception. I'm happy to accept suggestions if some of the current GalSimErrors ought to be switched to a new or existing subclass.
Some of the subclasses also inherit from one of the regular python exceptions like ValueError or NotImplementedError, which usually match the error that had been raised before these changes. So usually if people had been catching something like that, it will still work. Indeed I didn't bother changing many of the unit tests to the new error classes, leaving many as simply ValueError.
Finally, since I was touching so much of the code, including a lot of bits that hadn't been covered by the unit tests, I ended up writing a lot of additional tests to add coverage. I got kind of obsessive about this and ended up getting the repo up to 100% coverage. I know most of the rest of the developers aren't quite as anal about test coverage as I am, but I'd like to make this a requirement going forward for PRs to keep the 100% coverage. It's ok to add "#pragma: no cover" where necessary, but I think it will be a good goal to make sure all new code is covered and that we don't lose any existing coverage from any changes.
Oh, and while I was doing this, I realized that we still had a fair bit of cruft that was designed to work around various old versions of numpy or astropy or our own pre-2016 COSMOS catalog. I've removed all of these workarounds. For numpy and astropy, this is consistent with the requirements.txt file that we have now. And for the old COSMOS catalog, I still check, but now if gives an error if the catalog isn't the modern version rather than including the code to duplicate the calculations for the missing fields.