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

Add statement on explicitly returning nothing in control functions #4

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

BSchilperoort
Copy link

Ref csdms/bmi-example-julia#8

I added a general statement in the initialize docstring to avoid too much repetition.

@BSchilperoort
Copy link
Author

Working on the implementation in csdms/bmi-example-julia, I realize that initialize needs to return the model. I could have also looked at #1 of course.

@visr
Copy link
Member

visr commented Nov 11, 2024

Ha interesting that you chose initialize, since that is the odd one out in this case I believe, see #1 (which I now see you just found). Perhaps we could document that if a type is passed, we return the model, but if a model is passed, we return nothing.

I think I'd prefer the repetition in this case though, since I don't think we can necessarily expect everyone to read the initialize docstring first.

@BSchilperoort
Copy link
Author

Perhaps we could document that if a type is passed, we return the model, but if a model is passed, we return nothing.

I think consistency would be best here, i.e. always return the model.

In what case would you already have the model instantiated, and would want to re-use the struct to initialize it again? I'm having a hard time picturing that.

@visr
Copy link
Member

visr commented Nov 11, 2024

always return the model

Yes that's also fine by me.

In what case would you already have the model instantiated, and would want to re-use the struct to initialize it again?

Yeah I was thinking of some constructor that has empty arrays, which are then filled with initialize or something like that. But I don't really think it is needed, and I see bmi-example-julia, Wflow and Ribasim all only have the method on the type and not the instance. So perhaps a better way to resolve #1 would be to document that this is the only way: BMI.initialize(::Type{MyModel}, config_file)::MyModel.

@BSchilperoort
Copy link
Author

would be to document that this is the only way: BMI.initialize(::Type{MyModel}, config_file)::MyModel

To add these types, I'd have to define a MyModel struct. Shall I do that and also add types elsewhere?

@visr
Copy link
Member

visr commented Nov 12, 2024

Ah I meant to only document this. The MyModel struct example is not for BMI.jl but for users of BMI.jl.

@BSchilperoort
Copy link
Author

@visr I've added an example implementation to initialize that should hopefully clear up any confusion.

@BSchilperoort
Copy link
Author

I do see that in Wflow.jl and Ribasim you don't actually use the model's type, and that it's only passed so that julia uses the correct function. Maybe it's good to actually clarify that.

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