More informative errors

[BradGreig]

Hey @steven-murray, I have taken a first stab at updating the granularity of the error coding and modifying the error messaging to try and make them a bit more informative. As I mentioned I did away with the ParameterError thing entirely, turning everything into a FatalCError.

Obviously the long elif code is ugly, but just temporary for now. Let me know what you think.

In some places I added to the output error through LOG, in particular including information known from some recent failings with the PhotonCons option (under certain parameter combinations).

Also, I have added some error checks for some AstroParams, causing it to exit with an informative error as to why.

I'm contemplating adding a warning for the other parameters when they are outside the usual parameter ranges we have used in publications. The idea would be that if outside that range, we haven't tested if they'll pass/fail. So warn the user that this might be the case (that their parameter choice is outside of the ranges it has been tested on, but could still be fine).

[BradGreig]

Obviously at this point I don't really care if it passes/fails the tests as I probably haven't checked everything in detail regarding the ParameterError usage.

[BradGreig]

It's ok, I know why these are failing. I wanted the whole suite to run in case I messed something else up.

[steven-murray]

Hey @BradGreig, yeah I'll have a look, hopefully today (I'm pretty swamped though).

[BradGreig]

Hey @steven-murray thanks for the feedback. Yep, I knew that certainly wasn't the best way to do it, and knew you'd point me in the right direction (re. subclasses).

I agree with the planned hierarchy for errors. It's just that my preference is to have these exit by default. Certainly all for the option of suppressing/bypassing these. At some level I feel like the majority of these errors would be weird edge/corner cases of parameter combinations, which should be acceptable, it's just that they haven't been robustly checked. Thus, I feel the more that can be caught, reported and dealt with the more robust the code itself would become to these errors.

[BradGreig]

Hey @steven-murray, I tried implementing the subclass suggestion. I'm not sure this is exactly what you meant, as I still ended up with a large if/elif logic branch. I think I must be missing something...

[codecov[bot]]

Codecov Report

Merging #219 (0bdd060) into master (73548a8) will increase coverage by 0.20%. The diff coverage is 89.13%.

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
+ Coverage   84.61%   84.81%   +0.20%     
==========================================
  Files          12       12              
  Lines        2411     2444      +33     
==========================================
+ Hits         2040     2073      +33     
  Misses        371      371              

Impacted Files	Coverage Δ
src/py21cmfast/inputs.py	90.60% <71.42%> (-1.61%)	:arrow_down:
src/py21cmfast/_utils.py	91.79% <96.87%> (+1.12%)	:arrow_up:

---

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 5f522aa...0bdd060. Read the comment docs.

[BradGreig]

Hey @steven-murray thanks for the further feedback. I have implement these, and switched some of these errors to ParameterError. Let me know if there is anything further.

With these current errors I'm not entirely convinced that these should be able to be switched to warnings to continue an MCMC. Mostly because it is hard to know whether ignoring the problematic errors is impacting the posteriors. However, I guess that's up to the user whether that is a smart thing to do.

[BradGreig]

Hey @steven-murray, thanks for the comments. I have updated those accordingly. I was going to create the 21CMMC PR once this was finalised, but it seems you did it already.

Just a quick question. Is this just to be merged into the dev version? Basically, should it be lumped into that under a minor version release? Or should it just be kept separate. To me it makes sense that it should all go into the one minor version update, just wasn't sure of your plans.

[steven-murray]

Thanks Brad.

Yeah, this should be merged into master, and we'll cut a full 3.1.0 release ASAP. The next thing we release has to be a minor version (i.e. 3.1.0) because we have feature updates already in master. The update I did to the versioning a while back can really kick in after that -- then we can easily do smaller bugfixes after more serious feature updates by branching correctly.

Now that we have some really cool stuff in master, we should really push on getting 3.1.0 released.

On Tue, Mar 23, 2021 at 5:14 PM BradGreig @.***> wrote:

Hey @steven-murray https://github.com/steven-murray, thanks for the comments. I have updated those accordingly. I was going to create the 21CMMC PR once this was finalised, but it seems you did it already.

Just a quick question. Is this just to be merged into the dev version? Basically, should it be lumped into that under a minor version release? Or should it just be kept separate. To me it makes sense that it should all go into the one minor version update, just wasn't sure of your plans.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/21cmfast/21cmFAST/pull/219#issuecomment-805366100, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJWRXXDALMUNZQ72V4OOELTFEVFLANCNFSM4ZOFZJVQ .

[BradGreig]

Hey @steven-murray thanks for the clarification re. versioning. I might push over the next day or so to have the functions to estimate memory usage included in this same minor version release then. I'll let you know in a day or so whether or not that actually looks like happening.

[BradGreig]

This addresses #189. Also related to #134, however, that should probably remain open as a reminder of the need to keep thinking about how best to deal with this.