-
Notifications
You must be signed in to change notification settings - Fork 63
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
Remove native/internal LSTM model #600
Remove native/internal LSTM model #600
Conversation
f5ff67c
to
18fa8f1
Compare
18fa8f1
to
a457ec4
Compare
a457ec4
to
3ad8e2d
Compare
3ad8e2d
to
813f442
Compare
Once we get all these deprecations in place, we should ensure each is represented in a single commit, then document the commit and the deprecation in a changelog. |
Looks like this removes the C++ LSTM. Have y'all been running this version? If so, were you also running the Python version? Just out of curiosity. |
@jmframe No, we haven't in a long time [edit: the C version, specifically], that's part of the reason for the removal--I'm not even sure if it still works. The API around the formulations has been evolving and we kept having to come back to the old non-BMI models and do a lot of retrofitting--so for the time being we're removing them to streamline things, because that API is probably going to need to further evolve quite a bit for a while to deal with multiple discretizations (structured grid models, catchment models, network/routing modules, unstructured grid models...). I think there's space for non-BMI formulations in the future for various reasons including performance optimization (relevant to LSTM, probably), but the technical debt associated with the upkeep of things that weren't directly related to the capabilities we're being asked to develop was becoming costly. I'd like this to come back in the future after the internal APIs have stabilized more. |
Oh.. above was WRT the C version. The Python/BMI version is still being kept up by the formulations team and I think they're trying to build some model pieces to produce non-streamflow variables alongside it when it is used, for instance. |
Sounds good. That's what I figured. |
Builds on/requires #599
Additions
Removals
Changes
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist (automated report can be put here)
Target Environment support