Calling all ACF fitting algorithms from the make_fit binary

[ecbland]

This PR allows make_fit to call any of the following fitting algorithms: fitacf3, fitacf2, lmfit1, fitex2 and fitex1. This change was discussed briefly at our last WG meeting, and I thought it was worth having a look at the code before we discuss it again at the SuperDARN workshop. It was an easy modification and removes a lot of duplicated code.

Examples:

make_fit -fitacf2 20180323.0801.00.lyr.rawacf > 20180323.0801.00.lyr.fitacf2

make_fit -lmfit1 20180323.0801.00.lyr.rawacf > 20180323.0801.00.lyr.lmfit

make_fit 20180323.0801.00.lyr.rawacf > /dev/null
Please specify a fitting algorithm

As you can see in the last example, there will be an error if the user does not specify which algorithm to use, so there is no "default" fitting algorithm on this branch.

[ecbland]

Thanks for the productive discussion about this change during yesterday's DAWG meeting!

Here's my to-do list for this PR:

• Make separate command line options for each fitting algorithm Shorter name for the -fitting-algorithm command line option
• Update readthedocs tutorials to reflect the new behaviour of make_fit
• Add a very brief description (2 sentences + reference) for each fitting algorithm to readthedocs
• Add warning message to the documentation that fitex1 has not been tested thoroughly
• Add warning to documentation that fitex1 and lmfit1 do not include elevation angles
• Link the command line help to the readthedocs tutorial ("more information available here: ...")

[aburrell]

Potential shorter names:

• -fitalg
• -falg
• -alg
• -fit-alg (my favourite)

[egthomas]

Potential shorter names:

• -fitalg
• -falg
• -alg
• -fit-alg (my favourite)

Great suggestions @aburrell: -alg would be my vote from that list.

An alternative could be for each fitting algorithm to be associated with its own option (e.g., -fitex2, -fitacf3, etc) similar to how map_addmodel handles things, although would require a slightly different implementation.

[ecbland]

An alternative could be for each fitting algorithm to be associated with its own option (e.g., -fitex2, -fitacf3, etc) similar to how map_addmodel handles things, although would require a slightly different implementation.

@egthomas That's a good suggestion that requires a lot less typing from the user (and nice that it matches map_addmodel).

[mtwalach]

An alternative could be for each fitting algorithm to be associated with its own option (e.g., -fitex2, -fitacf3, etc) similar to how map_addmodel handles things, although would require a slightly different implementation.

@egthomas That's a good suggestion that requires a lot less typing from the user (and nice that it matches map_addmodel).

I think I would favour this as it's in line with map_addmodel.

[ecbland]

Thanks everyone for your input! This PR is now ready for review. I have modified the command line options based on @egthomas's suggestion, and edited the examples above to reflect that change.

I also updated the make_fit tutorial on readthedocs. You can preview the changes here.

[pasha-ponomarenko]

Hi Emma, Great effort in pushing this issue through! Just one thing I want to point out is that the mid-latitude echoes are affecting FITACF2 as badly as they affect FITACF3. You can see an example in this talk: https://superdarn2022.swl.ac.cn/viewReport.do?id=9245c903-9858-4d21-951b-ea27ea39fce6 This problem was essentially ignored until we stumbled across it while testing FITACF3, which created a wrong impression that the HMB issue is attributed solely to FITACF3. I believe that the Readthedocs text should be corrected respectively, i.e., by mentioning that this issue affects both packages or by just removing it from comments on FITACF3. What do you think? Cheers. Pasha I

…

________________________________ From: Emma Bland ***@***.***> Sent: Monday, June 6, 2022, 2:26 p.m. To: SuperDARN/rst ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [SuperDARN/rst] Calling all ACF fitting algorithms from the `make_fit` binary (PR #504) CAUTION: External to USask. Verify sender and use caution with links and attachments. Forward suspicious emails to ***@***.*** Thanks everyone for your input! This PR is now ready for review. I have modified the command line options based on @egthomas<https://github.com/egthomas>'s suggestion, and edited the examples above to reflect that change. I also updated the make_fit tutorial on readthedocs. You can preview the changes here<https://radar-software-toolkit-rst.readthedocs.io/en/feature-make_fit/user_guide/make_fit/>. — Reply to this email directly, view it on GitHub<#504 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABIRTSBJJEAOIHS3MXQQ2SLVNZNG5ANCNFSM5WSC6QJA>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[ecbland]

I've marked this PR as a draft since it will be easier to resolve the conflicts after merging #512.

If anyone is planning other changes that might create conflicts with this PR, please get in touch!

[egthomas]

@ecbland the tdiff changes are likely on a slower timeline (and require more discussion) than these changes, so I wouldn't be opposed to merging this pull request first and then cleaning up any conflicts on the tdiff pull request.

[egthomas]

So I took a crack at adding lmfit2 to the new make_fit on this branch, and there are a lot of conflicts with similarly named (but slightly different) functions and structures in the fitacf3 library.

@ecbland, do you see an easy way forward? Maybe lmfit2 could be added later?

[ecbland]

So I took a crack at adding lmfit2 to the new make_fit on this branch, and there are a lot of conflicts with similarly named (but slightly different) functions and structures in the fitacf3 library.

@ecbland, do you see an easy way forward? Maybe lmfit2 could be added later?

@egthomas Yep, I bumped into this problem as well :(
There must be a solution here, but I don't have one yet. I'm all in favour of adding lmfit2 later in a separate PR.

[pasha-ponomarenko]

I'm all in favour of adding lmfit2 later in a separate PR.

@ecbland , I second this.

[egthomas]

Thanks @ecbland and @pasha-ponomarenko. This looks good to me, although since this is a fairly significant restructuring maybe someone else should test again too.

[egthomas]

@ecbland well it's been a week, is it alright if I go ahead and merge this?

[ecbland]

@ecbland well it's been a week, is it alright if I go ahead and merge this?

@egthomas Yes, I think it's time to merge 👍