SuperDARN / rst

Radar Software Toolkit (RST)
https://superdarn.github.io/rst/
GNU General Public License v3.0
22 stars 18 forks source link

Tdiff calibration file update #512

Closed egthomas closed 1 year ago

egthomas commented 2 years ago

This is a draft pull request on behalf of the Tdiff Task Force with a preliminary implementation of "tdiff calibration files" and the C and IDL software needed to interface with them. From a user perspective, the important new features are the following:

By default, make_fit will still use the tdiff value from a radar's hdw.dat file. These changes should be compatible with both FITACF 2.5 and 3.0, although there are likely lots of conflicts with @ecbland's #504. The new tdiff field added to the fitacf-format files will also likely require some coordination with pydarn / pydarnio.

@aburrell has provided an example han.tdiff.dat file for testing purposes only, so please do not treat the values as fully calibrated (ie some are made up).


Examples for testing (added by @ecbland) Note the new syntax for make_fit

# Use tdiff from hardware file
make_fit -fitacf3 20150308.1401.00.han.rawacf > default.fitacf3

# Set fixed tdiff value
make_fit -fitacf3 -tdiff 0 20150308.1401.00.han.rawacf > tdiff0.fitacf3

# Use tdiff values from calibration file located in $SD_TDIFFPATH
make_fit -fitacf3 -tdiff_method 4 20150308.1401.00.han.rawacf > method4.fitacf3

The output fitacf files can be read using dmapdump or IDL. PyDARNio has not yet been updated to read the new tdiff field (this will be done soon)

ecbland commented 2 years ago

Nice work @egthomas :100:

there are likely lots of conflicts with @ecbland's https://github.com/SuperDARN/rst/pull/504.

Yes, probably. Why is this PR a draft? Just wondering if we can accelerate the testing of this PR, and then I can resolve the conflicts after this PR is merged. I'm very happy to test this one now if it's ready :smile:

egthomas commented 2 years ago

@ecbland thanks! I posted this as a draft for now to try to emphasize that this implementation is still very much open to feedback / changes suggested by the community. I think the PIs also wanted some time for their groups to be able to look at the code and test it, so I didn't want anything to get merged too quickly (ie similar to #455).

ecbland commented 2 years ago

@egthomas Ok, we should probably set a deadline then. Would 31st August be reasonable?

egthomas commented 2 years ago

I've just merged @ecbland's feature/make_fit branch into this one to try to get ahead of some of the merge conflicts once that branch is approved (#504). So please note that make_fit on this branch will now require one of the fitting options (eg -fitacf3, -fitacf2, etc) when testing.

ecbland commented 2 years ago

@egthomas Where are we at with this PR? Are the changes supported by the TDIFF task force or are they still under discussion?

egthomas commented 2 years ago

@ecbland I'm honestly not sure - I think the PIs were all notified a few months ago of these changes being available for testing, but I know this is also a busy time of year with teaching schedules.

As far as the Tdiff Task Force perspective, my understanding is that everyone is fully supportive of the substantive changes (ie new tdiff files, their contents, and their integration with make_fit, etc) while also being open to community feedback on the smaller details like the naming of command line options or documentation.

ecbland commented 2 years ago

@egthomas Thanks for the update! I will email the PIs today to request feedback and set a deadline. I'll make it clear that they can simply request an extension if more time is needed to review the changes. It's coming time to make a new RST release (which could be a major release, given the significant changes to the make_fit functionality.)

ecbland commented 2 years ago

To test this PR, I made a few fitacf files using various command line options:

# Use tdiff from hardware file
make_fit -fitacf3 20150308.1401.00.han.rawacf > default.fitacf3

# Set fixed tdiff values
# In the second example, I used the tdiff value from the calibration file (channel 1)
make_fit -fitacf3 -tdiff 0 20150308.1401.00.han.rawacf > tdiff0.fitacf3
make_fit -fitacf3 -tdiff 0.178 20150308.1401.00.han.rawacf > tdiff178.fitacf3

# Use tdiff values from calibration file located in $SD_TDIFFPATH
# For this calibration file, tdiff values were determined using method 4, so the second
#   example here (method 0) should revert to the values in the hardware file
make_fit -fitacf3 -tdiff_method 4 20150308.1401.00.han.rawacf > method4.fitacf3
make_fit -fitacf3 -tdiff_method 0 20150308.1401.00.han.rawacf > method0.fitacf3

# What happens if I provide both -tdiff and -tdiff_method?
make_fit -fitacf3 -tdiff 0 -tdiff_method 4 20150308.1401.00.han.rawacf > method4_tdiff0.fitacf3

Then I compared the tdiff and elevation angle values in each file, with the following results:

pasha-ponomarenko commented 2 years ago

elv_low is different in fitacf3 files. I think this is also expected---perhaps @pasha-ponomarenko can confirm? elv_low is the least square error in the elevation (note that the name of the field will change to elv_error when https://github.com/SuperDARN/rst/pull/472 is merged)

This is expected as the elevation error calculated in FITACF3 depends on the elevation angle value itself so that the same phase variance produces different elevation error values at different elevation angles arising from different tdiffs (https://agupubs.onlinelibrary.wiley.com/doi/full/10.1029/2018RS006638): image

ecbland commented 1 year ago

I've sent a reminder to the PIs about this PR, with a deadline of 18th October.

Before merging, the sample calibration file (tdiff.dat.han) should be renamed to tdiff.dat.tst

egthomas commented 1 year ago

Thanks @ecbland. Assuming this pull request is eventually merged, we may also want to consider following a similar approach for the tdiff.dat files as we've done for the hdw.dat files. Meaning we could keep the most up-to-date values in a separate repository and only copy them over to the RST once it's time for a new release. That way the Tdiff values aren't tied to the RST and are more easily accessible to other libraries such as the python code.

ecbland commented 1 year ago

There was no further feedback about this PR from the PIs, apart from expressions of support from some PIs who are members of the TDIFF task force, so I will merge this now. Thanks to everyone who has contributed to this important update!