Skip to content
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

Make Jacobian target store keys in usable manners #308

Merged
merged 2 commits into from
Feb 16, 2021

Conversation

riclarsson
Copy link
Contributor

Also add a few more Options to constants.h for dealing with Jacobian.
Some of these had spaces in them before. These are now removed since
enums cannot have spaces in their names. Would it be better to allow
spaces again? I think it's fine not to, and it would require a few
more steps to allow this.

riclarsson and others added 2 commits February 15, 2021 10:21
Also add a few more Options to constants.h for dealing with Jacobian.
Some of these had spaces in them before.  These are now removed since
enums cannot have spaces in their names.  Would it be better to allow
spaces again?  I think it's fine not to, and it would require a few
more steps to allow this.
Only enable options to turn off Fortran modules if Fortran support is enabled.
@olemke
Copy link
Member

olemke commented Feb 15, 2021

@riclarsson About the changes in CMakeLists.txt: I happened to discover the cmake "option" macro last week and was about to submit a patch similar to what you did in this PR. I added one commit from my branch to this PR that shows the options for disabling Fortran modules only if Fortran support is turned on.

@olemke
Copy link
Member

olemke commented Feb 15, 2021

No strong opinion on the spaces. I'm fine with not allowing them if it keeps the ARTS enum implementation code cleaner. Only downside is that it breaks old controlfiles once more. But the changed parameters names where probably not widely used anyway yet. If no one else has objections I'm happy with it.

@riclarsson
Copy link
Contributor Author

@olemke Great minds and all that. I also found out about this "option" last week. I didn't search more and just added the arguments because auto-complete was nice. Your version is better.

I also think we should be allowed to make breaking changes from now until version 3. Isn't the entire point of v.2.5 that we are cleaning up things added for features in 2.3 that we now consider we can improve? Keeping spaces in options might be a hard requirement though, because perhaps not all options are as clean as the two I just broke, and it would be good to just bite that apple now. (There's probably a bunch of MIT-licensed algorithms available that matches strings ignoring whitespaces, so I don't believe it's difficult to add such a thing here. Though I also prefer if options are kept simple without spaces since it forces simple names.)

@olemke olemke merged commit 24a4319 into atmtools:master Feb 16, 2021
@riclarsson riclarsson deleted the jacobian_target_change branch May 19, 2021 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants