-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Snopt7 now working with 7.7 and 7.6 APIs #7
Conversation
snopt7(bool screen_output = false, std::string snopt7_c_library = "/usr/local/lib/libsnopt7_c.so", | ||
unsigned minor_version = 6u) | ||
: m_snopt7_c_library(snopt7_c_library), m_minor_version(minor_version), m_integer_opts(), | ||
m_numeric_opts(), m_screen_output(screen_output), m_verbosity(0), m_log(){}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a check on the minor_version
parameter would be good here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what to check. Its unsigned. any number > 0 is valid. from 1 to 6 the old API will be triggered, 7 to inf will trigger the other API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that this should work for snopt 7.1 7.2 7.3 7.4 7.5 7.6 with the old API, for snopt 7.7+ with the new API.
If in the future they introduce a further change then we will have to catch it and have a further branching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's more like a boolean flag right now? Because phrased like that it seems like it is open to supporting newer api versions in the future, but clearly if the snopt guys put out a new API tomorrow and a user passes in minor_version
8, it will still crash.
Not sure what is the best course of action to be honest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps an enum
would be better? With an enum, you could have
enum class snopt_api: unsigned {
ver1, // Support up to SNOPT 7.4
ver2, // Support from 7.4 to 7.8
latest // Anything above 7.8
};
Then you could add ver3
between latest
and ver2
whenever the API changes again. Not sure, just spitballing here.
I think the minor_version
thing is a bit confusing because of its name, it seems like the user needs to fill in the correct snopt minor version but that's not actually the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right ok I think I see the point better now. This seems like a good place where one would use a warning system, if we had one in pagmo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue I have with enums is that they do not play nicely at the interface with python. Also, since we do not have all the libraries of all versions, nor a description of the API changes is easy to find, would not guarantee that this works with all the versions.
Its very confusing, I agree.
I did try to put in the docs warnings on these things. Also about possible crashes if the library version is wrong .... not sure how to make this properly but my guess is that its not possible since snopt7 is developed quite "old fashioned" and does not offer much support for a proper integration.
template <typename Archive> | ||
void serialize(Archive &ar) | ||
{ | ||
ar(cereal::base_class<not_population_based>(this), m_snopt7_c_library, m_integer_opts, m_numeric_opts, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the minor version member is missing from serialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch .... also adding it to the screen stream ... will do
Codecov Report
@@ Coverage Diff @@
## master #7 +/- ##
=========================================
- Coverage 96.55% 96.3% -0.26%
=========================================
Files 4 4
Lines 872 866 -6
=========================================
- Hits 842 834 -8
- Misses 30 32 +2
Continue to review full report at Codecov.
|
Tested. Here is the log of a working session. Both Snopt 7.7.1 and Snopt 7.2.4 were used (fortran). Note that the c library version is, instead, 2.2 and 2.0 respectively.