SIP80/SIP83 Remove multi-site support

[Alomir]

#80
#83

This PR:

• Removes multi-site support from SIPNET
  • Removed location from all places
  • Maintains compatibility with existing *.param files; the location column is ignored for those files
  • Simplifies "spatial" params
• For new param structures, removed obsolete members related to senstest/estimate parameters
  • Compatibility maintained here as well, extra columns are ignored
• Updates existing tests
• Adds new tests for reading old (and new) .clim and .param files

Note: I replaced spatialParams.h|c with modelParams.h|c (via git mv), but it appears here like a deletion and creation.

• This is not necessarily bad, as it may be easier to review modelParams on its own
• I am not at all locked into the name modelParams if anyone wants to suggest something else

[dlebauer]

I really appreciate your thorough use of validation, checks for exceptions, use of tests, and maintaining backwards compatibility! A am impressed that you can keep all of this logic in your while dissecting and excising this code.

• I am not sure if this was intentionally in this PR vs SIP78 Convert switches to run time options #96, but I think that it is safe to remove the moisture_bwb and moisture_pm functions (that were commented out in this PR). When removing functionality like this I'd just make it clear in the changelog when and why it was done to help facilitate discovery and resurrection in the future for anyone so motivated.
• We discussed deprecating src/utilities/subsetData.c in which case this is just one more reason to do so - although the utility only works for files with one location, it expects there to be a location column and then confirms that there is only one location.
• As suggested on Slack I only skimmed the tests to make sure they make sense!

[dlebauer]

Is there or should there be a standard set of 'reference' input files (e.g. that we can link to in docs?) I like the idea of such files being used in tests (like this one) to make sure they are up to date.

Also:

Suggested change

	!!! Modified from the original version by Bill Sacks ([email protected])
	!!! Modified from Bill's version by Mike Longfritz (@Alomir)
	!!! Modified from the original version by Bill Sacks ([email protected])

[Alomir]

I like this idea! I'd go so far as to suggest a /tests/sipnet/reference_inputs directory for them (or some such) - a central place for a suite of input files. I think this would be best as a separate ticket?

[Alomir]

Created issue #102 for this (I put it in with the more general unit test revamp that's needed once this PR gets in)

[dlebauer]

I don't see where this file is used in the codebase - should this function be documented somewhere? should it have a #!/bin/bash and .sh extension?

[Alomir]

All good questions!

• I wrote (and used) this utility to trim the location columns from a lot of input test files; I thought I'd add it here in case we wanted it for similar use. (I typically don't expect scripts in a /tools directory to be used in the codebase directly)
• Yep, it should be documented somewhere, though I'm not sure if it would ever be for more than dev use
• And yep, it should have a .sh extension. I often don't do that with my own scripts, but that's a personal preference; having the extension is the usual expectation.

[dlebauer]

Thanks for updating this. Although simple, it seems like it could be a handy utility for someone who wants to update their old files... perhaps mentioning it in release notes would be sufficient?

[Alomir]

Done.

[dlebauer]

It is great to see Doxygen docs, thank you! However, in this case, the duplicate what is in docs/parameters.md. It seems we should keep a single version that can be cross referenced.

In this specific case, I think that the end user is the key audience and thus one of the docs/*md files might be best (currently parameters.md but I suggested a separate file for inputs & outputs in #99).

[Alomir]

Yes, I've been a bit worried about doc duplication; I have a few comments like "see sipnet.h" in sipnet.c for that exact reason.

I agree that this comment block should go in favor of the markdown version, wherever that ends up. We should make sure to specify where that is in the code here.

[dlebauer]

It is already in

sipnet/docs/parameters.md

Line 439 in f46e73d

### Agronomic Events

though this will eventually move (#99)

[dlebauer]

? is this correct?

Suggested change

	! LOCATION is obsolete; this value is ignored
	! LOCATION is obsolete and this value is ignored; maintained for backward compatibility

[Alomir]

Actually, I think this should just get deleted (both here and in /tests/smoke/sipnet.in)

[dlebauer]

Works for me!

[Alomir]

• I am not sure if this was intentionally in this PR vs SIP78 Convert switches to run time options #96, but I think that it is safe to remove the moisture_bwb and moisture_pm functions (that were commented out in this PR). When removing functionality like this I'd just make it clear in the changelog when and why it was done to help facilitate discovery and resurrection in the future for anyone so motivated.

I meant to un-comment those out before committing - I was only checking here that they actually were not needed. I'd like to restore them here, and let them get dealt with more appropriately in that other ticket

• We discussed deprecating src/utilities/subsetData.c in which case this is just one more reason to do so - although the utility only works for files with one location, it expects there to be a location column and then confirms that there is only one location.

Agreed. If no GSOC person picks that ticket up, I'm likely to do it soon so it stops nagging at me :-)

• As suggested on Slack I only skimmed the tests to make sure they make sense!
  👍