SuperDARN / rst

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

CDF dependencies #17

Closed egthomas closed 7 years ago

egthomas commented 7 years ago

What are everyone's thoughts on removing CDF functionality from the RST? Its main purpose is reading a certain type of ACE/WIND/IMP file for the map_addimf routine, and for plotting some legacy ISTP stuff. Does anyone still rely on this? By keeping it in the RST we are creating a few extra steps for users when they want to download and compile the software (go get CDF library, modify CDF_PATH environment variable, etc).

pasha-ponomarenko commented 7 years ago

I think this is reasonable, but I recall that we need to know IMF for applying the statistical model. So, could you clarify what is going to happen to reaing the IMF data if we remove the CDF capablity? Would it be possible to perform proper fitting/maping without that "certain type" of data files?

egthomas commented 7 years ago

Users can either set the IMF values directly using command line options (-bx, -by, -bz) or by providing an ASCII text file. I actually don't even know if the files being posted to CDAWeb are still compatible with the -ace or -wind options; that would be something to test.

I expect this (along with plotting) may be one of the things we need to poll the broader SD community on whether it is a feature that is still used.

pasha-ponomarenko commented 7 years ago

So, let's wait and see!

egthomas commented 7 years ago

I tried using istp_plot with the following CDF files from CDAWeb for the date 20150101 with no success (received a "Nothing to plot." error message):

Similarly, trying to use those files with the map_addimf routine just adds zeros to the Bx/By/Bz fields. It actually wasn't even obvious how to set up the directory structure after modifying the ISTP_PATH environment variable in superdarn.bash without reading the documentation for istp_plot or map_addimf.

I could be using the wrong data level file (h0? h1? h2? k0? k1?), or maybe the contents/format of the CDF files have changed since these routines were written, but either way I am much more inclined to drop this functionality entirely.

egthomas commented 7 years ago

Tomo has pointed out that I was missing the "year" subdirectory within the $ISTP_PATH/ace or $ISTP_PATH/wind directories so my previous comment can be disregarded. At a minimum though the documentation for those routines should be updated for clarity.

egthomas commented 7 years ago

After some experimentation with the various routines that use the CDF functionality I've softened my stance and can see the value in keeping them in the RST. One possible solution to the extra compilation step would be to include the CDF library with our distribution, although that opens up the can of worms of distributing others' software. Which is actually something we're already doing with Geopack and raises the related issue of whether we should continue to do so.

pasha-ponomarenko commented 7 years ago

I am not sure what is the proprietary side of this -- I guess that these packages are free -- but it definitely adds extra trouble if you want to have both packages intergated into RST. From my dummy propspective, I assume that this would require some sort of automated version control and update as well. However, is there any viable alternative? What are the actual parameters required for mapping? Are these By and Bz only? Can we just download them as ASCII from, say, SuperMAG and distribute with RST instead of distributing the CDF library? These might be silly questions, but...

egthomas commented 7 years ago

For now the only model in the RST (RG96) requires By and Bz, however existing (PSR10, CS10) and future (TS18?) models will also require/accept Vx, dipole tilt, and possibly Kp as input parameters.

Everyone probably has their own favorite solar wind data set (ACE, DSCOVR, OMNI, etc) so I think we want to maintain some flexibility there.

kkotyk commented 7 years ago

If CDF is remaining in the package, I think the CDF path environment variables should be updated. The default CDF install path is /usr/local/cdf, so can we set this to the default CDF path?

egthomas commented 7 years ago

Yes that should be fine. Some people may still need to modify the CDF_PATH environment variable, but if they were capable of installing it in a non-default directory they should be able to figure out how to modify the base.bash file.

kkotyk commented 7 years ago

In the installation of CDF, you do specify the installation directory for CDF, but location they use in their example is the one I listed, and I feel most people will use that directory. I agree with your points though.

egthomas commented 7 years ago

Any objections to changing line 18 of .profile/base.bash to the following per @kkotyk 's suggestion?

export CDF_PATH="/usr/local/cdf"

I can go ahead and modify base.tcsh at the same time to make it comparable.

ksterne commented 7 years ago

Running through the CDF install a few times, I didn't recall there was a default of an example one. The /usr/local/cdf path does make sense though. This change sounds good to me.

kkotyk commented 7 years ago

If you look at the Help.install file in the cdf stuff you will see it. make install installs to current directory and their example of installing to a different system wide directory is /usr/local/cdf.

egthomas commented 7 years ago

Unless there are any objections, I think for now we can leave the CDF functionality alone and consider this issue closed.