Feature/interface for mean tidal potential 100 #434
Conversation
|
@jo11he I closed the other issue for this, since it was for pre-1.0 consistency, which we don't need anymore :) I think we can merge this PR once there's a unit test that the mean tide is properly subtracted during the estimation as well. Do you have one already? |
| const int maximumDegree, | ||
| const int maximumOrder ); | ||
| const int maximumOrder, | ||
| std::map< int, std::vector< double > > meanCosineForcing = { {0, {0.0}} }, |
There was a problem hiding this comment.
Instead of initializing a specific map, initialize to empty map std::map< int, std::vector< double > >( ), which will not allocate needless memory (and makes the check for whether mean tides are added easier)
|
|
||
| // if cosine mean forcing terms are empty map (default), set map values to zero | ||
| if (meanForcingCosineTerms.empty()){ | ||
| for (const auto& [key, vec] : loveNumbers_) { |
There was a problem hiding this comment.
The const auto& [key, vec] requires C++17, which is not (yet) the default of Tudat compilation, could you modify this?
| void setMeanForcingCosineTerms( const std::map<int, std::vector<double>>& newCosineTerms ) { | ||
|
|
||
| // Check same number of keys | ||
| assert(newCosineTerms.size() == meanForcingCosineTerms_.size() && "Error when setting meanForcingCosineTerms in BasicSolidBodyGravityFieldVariationSettings. Keys of newCosineTerms argument must match with existing love number keys."); |
There was a problem hiding this comment.
Use exceptions (like you already have) instead of assert
| } | ||
|
|
||
|
|
||
| void massageMeanTermsIfDefault(std::map< int, std::vector< double > >& input, const std::map< int, std::vector< std::complex< double > > >& loveNumbersReference) { |
There was a problem hiding this comment.
This is now done 'in the loop', allocating memory each time step. I think this can be checked and done once (?)
There was a problem hiding this comment.
I think this comment refers to the call to massageMeanTermsIfDefault?
This function is only called in std::pair< Eigen::MatrixXd, Eigen::MatrixXd > calculateSolidBodyTideSingleCoefficientSetCorrectionFromAmplitude( ), which is the overload using const std::map< int, std::vector< std::complex< double > > > loveNumbers as input. This function is only called in the unit tests.
As such, it does not go through any "preparations", it is just called out of the blue, for a given state of the environment (so also not "in-the-loop"). Without the "preparations" it is also impossible to adjust the format of the 0.0 meanTidalForcing defaults to the format of the loveNumbers input, which can be of arbitrary std::map< int, std::vector< std::complex< double > > > format. Therefore, this adjustment needs to be made inside the body of this function.
In short, I think this function was never intended to be used by a user, not even the C++ user in propagation, only as a facilitator in unit tests.
There was a problem hiding this comment.
Sounds good, no need to change this
| } | ||
| } | ||
|
|
||
| } else { |
There was a problem hiding this comment.
why not do this in a single piece of code, rather than the if/else?
|
@jo11he I added a couple of comments into the code, and extended the tests for the Love number partials. There are some failures in the test that indicate some things are not yet as they should be:
The last one is particular strange, since it these Love number partials should not be affected at all by your code. I did very little investigation, so it's also possible that the problem is in the test, not the functionality :) |
|
UPDATE: I have not found the error, but I have a good lead. On of the parameter where the partials go wrong is set up (on purpose, and after 10 years it's paying off by finding a bug!) as: [k22,k20,k21] when I modify this to setting it up as [k20,k21,k22] it all works fine Since the same is done for the mode-coupled Love number partials, I suspect this PR incorrectly modified a book-keeping aspect in the parameter/partial. To be investigated further :) |
|
I'm on it |
DominicDirkx
force-pushed
the
feature/interface_for_mean_tidal_potential_100
branch
from
f5b3026 to
12acb9a
Compare
…ub.com:jo11he/my-tudatpy into feature/interface_for_mean_tidal_potential_100
|
I think this is looking good now! |
2 participants
Feature branch implementing user option to subtract mean tidal forcing from solid body tide model.
Unit test implemented only in unitTestEnvironmentModelSetup.cpp, not unitTestGravityFieldVariations.cpp.
Unit test for partials is pending.