Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

set_value argument name: src or values #6

Closed
visr opened this issue Feb 12, 2020 · 5 comments
Closed

set_value argument name: src or values #6

visr opened this issue Feb 12, 2020 · 5 comments

Comments

@visr
Copy link
Contributor

visr commented Feb 12, 2020

I just noticed in

bmi-python/bmipy/bmi.py

Lines 394 to 407 in 0fcca44

def set_value(self, name: str, values: np.ndarray) -> None:
"""Specify a new value for a model variable.
This is the setter for the model, used to change the model's
current state. It accepts, through *src*, a new value for a
model variable, with the type, size and rank of *src*
dependent on the variable.
Parameters
----------
var_name : str
An input or output variable name, a CSDMS Standard Name.
src : array_like
The new value for the specified variable.

That the argument name values does not match the src in the docstring and the src on the BMI website.

Since in Python these are not strictly positional but can be addressed as keyword arguments as well, I didn't just want to change it.

@mdpiper
Copy link
Member

mdpiper commented Feb 13, 2020

@visr It should be src, to match bmi.sidl. Thanks for spotting this error!

@mdpiper
Copy link
Member

mdpiper commented Feb 13, 2020

Fixed with 17d7d72.

@mdpiper mdpiper closed this as completed Feb 13, 2020
@visr
Copy link
Contributor Author

visr commented Feb 13, 2020

Ok thanks! I assumed it should be src. What I meant was that 17d7d72 breaks code of people that used keyword agument syntax like set_value(name="myname", values=np.zeros(3)) as opposed to set_value("myname", np.zeros(3)), since values is now not an argument name anymore. Not sure if that is an issue in practice though, you can probably judge that better.

For your information, the main reason I was carefully reading it was because I used it as a basis for https://github.com/Deltares/BasicModelInterface.jl. Not sure to which extent you would be interested in a Julia BMI specification next to the C, C++, Fortran and Python ones. As I mention in the readme it probably still needs some implementations to prove itself. But perhaps it's good you are aware of its existence. If you'd want to adopt it eventually I'd be happy to transfer the repo to the CSDMS org.

@mcflugen
Copy link
Member

@visr This is great! @ethan-pierce is also working with BMI for Julia. I believe his repository is private at the moment but you two should compare notes sometime.

@visr
Copy link
Contributor Author

visr commented Feb 13, 2020

Great, thanks for letting me know. @ethan-pierce, feel free to create an issue on the BasicModelInterface.jl repository to discuss anything, or if you prefer you can drop me an email as well. I didn't register BasicModelInterface.jl as a package yet, so we could also go with what you worked on.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants