SuperDARN / rst

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

Binary help messages #113

Closed ksterne closed 6 years ago

ksterne commented 6 years ago

As found in #111, there are a few binaries that could use some work to make them a little more user friendly at the command line.

Test_fitex2 also doesn't behave as usual, though if you try to give it a -new it at least gives a warning, but --help doesn't offer anything (outside of this PR, but I'll start an issue to note it). Same with test_lmfit.

lagfr_fix doesn't seem to have any help notation at all.

make_fitex doesn't seem to have any help notation at all. Same for make_fitex2 and make_lmfit

I get that these were more recently binaries that were written by folks that aren't with SuperDARN anymore, but if they're in our package, we should acknownledge where we need to do some work.

egthomas commented 6 years ago

Personally I think lagfr_fix should be removed entirely.

pasha-ponomarenko commented 6 years ago

@egthomas, is this code allows for adjusting the first range gate? I recall finding out in 2011-12 that the horizontal gaps in the range time plots are due to inaccurate timing between the emission and sampling. A procedure to estimate the delay is implemented on VT website, and this code might be developed in order to re-write the correct value into the par structure of an offending rawacf file. This problem became rather widespread with switching to digital radars so it requires a more general solution like introduction of a first range gate offset in the hdw files (analogous to tdiff).

ksterne commented 6 years ago

I think there was a different set of code that was used to fixed data from 2011-2012, but I imagine that the 'lagfr_fix' code here works much the same. Why do you think that it should be removed though @egthomas?

egthomas commented 6 years ago

My main concern stems from the fact that the routine is entirely undocumented. Who wrote it? (It wasn't actually distributed in pre-VTRST3.5 releases of the RST so I doubt it was Rob.) What is its purpose? When should it be used? What do the command line options control? Why does it only modify lagfr/frang and not any other operating parameters in the prm block?

If the code is meant to address a specific issue with the data then I'm not aware of that issue being documented anywhere online (maybe in a workshop presentation or mailing list discussion? neither of which are publicly accessible).

In other words, users shouldn't have to dig into the source code to understand what a routine does if we're distributing it to the general community - we should apply some level of quality control before allowing code into the general software distribution. Things shouldn't get a pass just because they were added before this working group started maintaining the RST.

ksterne commented 6 years ago

I can certainly understand your concern. But then should we remove code that effort was put into writing just because we don't understand how to use it? And then when the same issue comes up again, we'll put the same effort into writing that code? Your concern seems like removal of code because we're not willing to learn and understand it for ourselves and properly document it.

I get that it's way easier to just hit 'delete' than to do anything else. But what harm is it causing? Is it breaking compilations? Is it a burden of filesize in downloading? How many issues here are related to people having questions on how to use it?

I also get that deleting isn't so terrible in git and for this codebase as there are a few backups (at least VTRST3.5?).

Should we also remove the other binaries that are listed here? A quick look at make_fitex2 and it looks just as uncommented/undocumented.

egthomas commented 6 years ago

For make_fitex2 and make_lmfit there is at least AJ's dissertation and Radio Science paper which describe the different fitting algorithms. It's still not clear to me whether lagfr_fix is something a user needs to apply to data they've obtained from one of the mirrors, or if it's used by the data providers before posting rawacf data to the mirrors, etc.

ksterne commented 6 years ago

True enough that there's peer-review papers that describe the algorithms. But do the papers outline where things are happening? It's slightly better, but without access to these and just looking at the code, it's not well documented. There's also no connection from the code back to those papers.

I wouldn't imagine many will need to use lagfr_fix but I'm not sure who the user might be. If the user's gotten ahold of data prior to it being corrected and it's quicker for them to run the code than to redownload the files, then that could be a scenario where it's used by a user. This is assuming the code does what I'm thinking it does with modifying a rawacf file's lagfr parameter.

egthomas commented 6 years ago

For now I'll leave it at this: if lagfr_fix didn't already exist in the RST and someone were to open a pull request to add the lagfr_fix code in its current state, would it be approved? (ignoring the glacial rate at which pull requests are handled)

pasha-ponomarenko commented 6 years ago

Guys, I am somewhere in Australia effectively without internet until next Monday... Let's leave it until then.

Cheers, Pasha


From: Evan Thomas [notifications@github.com] Sent: 26 February 2018 09:00 To: SuperDARN/rst Cc: Ponomarenko, Pasha; Comment Subject: Re: [SuperDARN/rst] Binary help messages (#113)

For now I'll leave it at this: if lagfr_fix didn't already exist in the RST and someone were to open a pull request to add the lagfr_fix code in its current state, would it be approved? (ignoring the glacial rate at which pull requests are handled)

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/SuperDARN/rst/issues/113#issuecomment-368530651, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AFEZyFIYnM2YHbbRqfxbELZ3MsiN8V-Dks5tYsb7gaJpZM4Q24ba.

pasha-ponomarenko commented 6 years ago

OK, I am back to my office (and stable internet connection!). Still a bit dizzy from a 16-hr jetlag -- landed in Sask only last night.

I think it is necessary to open a discussion item on the range offset and issues related to its detection (some stuff has been implemented in the VT online tool) and treatement (lagfr_fix was designed for that purpose).

Another thing is that we may benefit from yet another telecon in order to address similar issues as well as to clarify our current priorities and future actions.

Just let me know what you think, guys!

ksterne commented 6 years ago

I think that's a good idea about the range offset and lagfr_fix code being a separate issue.

I'm good to do a telecon here sometime soon. @pasha-ponomarenko, do you want to setup a doodle to see when is good for a DAWG meeting? Probably would be good to do that to layout a 4.2 release timeline/schedule/goals if we think that's necessary.

pasha-ponomarenko commented 6 years ago

@ksterne, please set up a doodle. ;-) I would think about having it the next couple of weeks, preferably during the last week of March because I need to get back to speed with several things here, including revision/testing of outstanding pull requests.

With respect to lagfr_fix, I can insert some comments on its supposed purpose, and then we can place it somewhere in a safe place for the time being.

I will formulate the essense of the range offset problem and post it as a separate issue.

egthomas commented 6 years ago

With respect to the original issue, the test_fitacf, test_fitex2, and test_lmfit binaries don't have much if any documentation because they were written by AJ specifically for the ACF Plot and Fit Tool on the VT website (http://vt.superdarn.org/tiki-index.php?page=ACF+Plotting+Tools). These binaries write a custom output file which is only read by a few of the VT-specific IDL routines which were removed from the RST over a year ago in #9.

pasha-ponomarenko commented 6 years ago

So, you suggest that we can remove these three binaries and just make sure that their copies are stored somewhere at VT?

egthomas commented 6 years ago

I could, but I think I know what the response will be.

pasha-ponomarenko commented 6 years ago

Mind you, @ksterne wrote before:

I also get that deleting isn't so terrible in git and for this codebase as there are a few backups (at least VTRST3.5?).

so I think we will be able to resolve this at the telecon in no time at all. ;-)

ksterne commented 6 years ago

As I see it on removing the test_* packages: Pro: We remove some older custom code that probably won't be useful for anyone but isn't really breaking compliation or hurting any other piece of code (aside from requiring the older fitacfT library, more on this later). Con: Breaks a tool on the VT SuperDARN website that gets a decent amount of traffic. Creates work for me to redirect code to execute a binary in an older repo.

I do recognize that at least the test_fitacf tool is using the older fitacf library and needs to be updated. But that's not easy to do without adding some print statements to the current fitacf library. It will be hard for me to find the time to restore things here, let alone update things. Certainly will take whatever resolution we get out of the telecon.

egthomas commented 6 years ago

The fitacfT library can cause issues when compiling fitacf.2.5 - see 79d6ac2

pasha-ponomarenko commented 6 years ago

Con: Breaks a tool on the VT SuperDARN website ...

So, currently these routines cannot be installed/used independently? This is a new bit of information for me.
I guess, the general line should still be to remove (eventually!!!) all site-specific bits of code from RST, but breaking the VT website is a bit too high price for that. Let's talk about it next Thursday(?).

egthomas commented 6 years ago

Addressed by #154.

ksterne commented 6 years ago

Has it though? make_fitex, make_fitex2, make_lmfit, and lagfr_fix still don't offer any output for --help option. I'm not sure if I went through all of the binaries or not either to see if these were the only ones.