Fix up rescale_I_model_3D.py

[AlecThomson]

Closes #131 and adds some improvements discussed there.

We really need to add tests - I've added a stub but I'm not sure of the expected inputs / outputs (and don't have a demo to hand).

If we can it'd be better to unit test each function with input/output rather than end-to-end test. But something is better than nothing!

[Cameron-Van-Eck]

I've started some quick and dirty tests of the "helper" tools (everything invoked on the CLI as rmtools_<helper>), although I might not have time for more than checking that they run to completion.

At risk of coming across as slightly snarky (I'm too jet-lagged to word this properly) -- maybe we should hold off on any major changes (e.g., changes to NamedTuples) until we have a slightly more thorough test suite and/or a strategy for making sure that changes get propagated to all the various sub-tools.

I should have some tests of the rescale code from when I built it, so I'll try to dig those out and test the fix. Maybe build a proper test if I have time later today.

[Cameron-Van-Eck]

I'm going to reject this PR for a couple of reasons:

• Mainly, I think that a bug fix should be the minimal changes needed to solve the bug; this PR goes much further than that. Larger changes should be done more methodically, and submitted to the dev branch first rather than direct to main.
• This PR makes significant changes that aren't consistent with the rest of RM-Tools. E.g., changing from import astropy.io.fits as pf to from astropy.io import fits. This is a purely stylistic choice, but there's no point making an arbitrary change as part of a bug fix. If we want to change styles, we should do it consistently across the whole package. Changing from os.path to pathlib falls into the same category, as do the type hints.
• This PR makes changes that, while neat, aren't relevant to the bug fix. E.g., parser.add_mutually_exclusive_group() -- I didn't know you could do this, and it's cool, but let's not push new things like this directly without more testing/thinking/discussion. This is why I wanted to have a dev branch.

I have separately pushed the core of your fix (fixing rescale_I_pixel to use the FitResult NamedTuple), since that's enough to fix the immediate bug.

[AlecThomson]

I've got a couple of thoughts here, which I'll try to articulate.

I don't see why it was necessary to close the PR overall. In general, I don't think there is a strict necessity to only change a single thing in any given PR. I also think incremental changes are useful in any long-lived code. They help to future-proof new parts of the code, when going back into older sections would be a mammoth task.

Whilst this PR may seem a little verbose at first glance the actual changes here, in my opinion, are relatively small. I think that this bug could have been prevented by (along with tests) making use of some of the more modern python features available e.g. type hints. In the longer term, we can confirm our understanding of function behaviour with unit tests.

I'm all for having rules on how we go about contributions. However, it's not like such rules have been adhered to historically, nor are these actually written down anywhere and agreed to by the major contributors.

This PR makes significant changes that aren't consistent with the rest of RM-Tools. E.g., changing from import astropy.io.fits as pf to from astropy.io import fits

That was pure habit on my part having ported some older pyfits code. This could have been easily changed in this PR upon feedback. Again, there isn't any existing style guide or rules in place for this kind of thing.

Changing from os.path to pathlib falls into the same category

No such change is in this this PR.

as do the type hints.

I disagree. Type hints are just that in Python, hints. They make no difference in the execution of the code. They do, however, make life as a developer much easier for finding, fixing, and preventing bugs. I really needed here them to help me make sense of the flow in this mode.

On NamedTuples, in this case we can also see they make no runtime difference when tuples were previously (implicitly) returned. See line 225 -> 254 (in PR):

covar, coeff, old_freq, new_freq = data

it remained the same. The difference was that my IDE was then able to tell me the types of these variables.

If you take the time to read through this PR you can see that none of the actual logic is changed. Instead, I was trying to help future proof potential issues that can be caused by some thing things in this script. Things like:

• Single letter or hard-to-read variables
• Mixing of command-line and 'business' logic
• Hard-coded magic numbers (especially in string parsing)
• Exception names (which get reported to the user on the error)

Whilst these changes do indeed go further than the bug itself, my hope was to help improve the overall quality of the code here and prevent future pains.

This was also just a PR, no actual changes were implemented 'in production'. Indeed I was hoping to implement some more tests in this PR to make sure we keep on the right track.