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

solar constant #19

Closed
SallyDa opened this issue Jan 12, 2018 · 12 comments
Closed

solar constant #19

SallyDa opened this issue Jan 12, 2018 · 12 comments

Comments

@SallyDa
Copy link
Contributor

SallyDa commented Jan 12, 2018

Hi,
I am working on a simple model using RRTMG thanks to the nice python wrapper you have written :) I would like to use it with a constant solar constant (no variability), but a different value to the standard one. I am currently using CliMT version 0.9.1 and have achieved this by changing the value in the init of the RRTMGShortwave class. I notice you have changed this in the development version of CliMT.
Please could you make a more user friendly way of changing this value? The aim is to make our model easy for other people to use, so that they run our model with CliMT installed but without needing to edit it.
Thanks,
Sally

@JoyMonteiro
Copy link
Member

JoyMonteiro commented Jan 12, 2018

Hello @SallyDa,

I'm glad that you find this useful! Sorry for the change in API, but climt is under heavy
development right now, and we believe this change will make models more consistent
in the future.

You could use the new API to change the solar constant easily:

climt.constant_library.modify_constants_in_library({
'stellar_irradiance': {'value': 200, 'units': 'W m^-2'}})

You should add this line before any components are created.

This is a little longer than the previous API. But the advantage is that all components which use
the solar constant will automatically use the new solar constant.

Also, since we intend climt to be more general in the future, we use the name
'stellar irradiance' instead of 'solar irradiance'. Sun is also a star, after all ;)

@mcgibbon
Copy link
Member

mcgibbon commented Jan 12, 2018

Oh my gosh @JoyMonteiro Joy we need a cleaner, simpler API than that, and it should be accessible from Sympl also. Perhaps something like sympl.set_constant('stellar_irradiance', value=200, units='W m^-2') that is also available as climt.set_constant would be much cleaner, I think.

In general though @SallyDa, we're hoping the new way of having the constants set on the CliMT/Sympl level instead of as keyword arguments to each component should prevent users from writing code that has strange bugs that are hard to identify (by resulting in different components using different constants). It should also be just as easy, or easier, to set the constants (since you only have to do it once, and not for each component).

@JoyMonteiro
Copy link
Member

Yes, that is kind of unwieldy!! I think it should be easy to add a climt.set_constant right away, and once we figure out the climt/sympl constants naming convention, it could be done at the sympl level itself.

@mcgibbon
Copy link
Member

mcgibbon commented Jan 12, 2018

OK, I've done it on the Sympl level in the current master. @SallyDa, you could either get the latest master and modify it there, or you can use a line like sympl.default_constants['solar_irradiance'] = sympl.DataArray(200. attrs={'units': 'W m^-2'}), which will work with your current version of Sympl and CliMT (or the line suggested by Joy). Joy, it's your choice whether you want to import set_constant into CliMT or encourage users to call sympl.set_constant (there are pros and cons of either).

@mcgibbon
Copy link
Member

mcgibbon commented Jan 12, 2018

Also @JoyMonteiro, the function you have right now should probably be called set_constants_from_dict. "set" because I'm guessing it will let you set new constants, and not just modify existing ones, and without the "in_library" because it's obvious that if you're calling climt.set_constants_from_dict it's library-wide. If it only modifies (which is actually a good thing, since it prevents typos), then "modify" is good.

If you want to do "modify_constant" instead of "set_constant", you should check the constant is in the default_constants dict when you wrap the Sympl function.

@JoyMonteiro
Copy link
Member

JoyMonteiro commented Jan 12, 2018

@SallyDa just to clarify, with the latest master of sympl, you can do
sympl.set_constant('stellar_irradiance', value=200, units='W m^-2'),
and you can alternatively do
climt.set_constant('stellar_irradiance', value=200, units='W m^-2')

@mcgibbon, I think for now I will import set_constant into climt, and your name suggestions
have also been incorporated.

@SallyDa
Copy link
Contributor Author

SallyDa commented Jan 15, 2018

Great, thank you both :)

@JoyMonteiro
Copy link
Member

Glad to be of help! If you think this solves your concern, I will close the issue.

@SallyDa
Copy link
Contributor Author

SallyDa commented Jan 16, 2018

Hey,

Thanks. It's updating the dictionary, but in the place I am using it, it is still not taking the updated value. I think the problem is in the following lines:

if use_internal_solar_constant:
self._solar_const = 0
else:
self._solar_const = get_constant(
'stellar_irradiance')

And I think it should be:

if use_internal_solar_constant:
  self._solar_const = get_constant(
              'stellar_irradiance')
else:
  self._solar_const = 0 

But the doc strings for solar_variability_method are quite confusing, so maybe you can check :)

@JoyMonteiro
Copy link
Member

Yes, the name I used for the argument is confusing; and the behaviour of the component is also confusing. I have fixed both in the latest commit. I think it should work now as you expect! let me know.

@SallyDa
Copy link
Contributor Author

SallyDa commented Jan 18, 2018

Okay that makes sense, and it's working now as I wanted. Thanks for all the help :)

@JoyMonteiro
Copy link
Member

Great! Closing.

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

No branches or pull requests

3 participants