Closed jsadler2 closed 2 years ago
Hey @jsadler2, I'll try to get to reviewing this later today, but wanted to comment quickly on the impetus behind this PR over #146. Specifically in you're last comment:
@janetrbarclay mentioned that it would be ideal if (paraphrasing here) if we can add the new functionality without losing the existing functionality. I think that is wise. I think there is a pretty straightforward way to do that. I will revise this PR to do that.
I'm not sure I agree that we should keep the option to pass in model_type
over just passing a compiled model. River-dl is already a large and complex workflow repository. I'm of the mind that in simplifying the training routine we should also be simplifying the codebase. Keeping functionality for both seems like it's adding unnecessary lines of code, and while it's more flexible, it also creates redundancy and requires additional explanation of the various ways you can use the repository. For new users, and to maximize generalizability, I think it's more helpful to have a standard high level workflow that we can explain in detail in the Readme
. This seems like a high level conversation that might influence how we review the PR, so I wanted to throw it out there before @janetrbarclay or I got started. Thoughts?
My comment was intended more that we should be sure the code can achieve the same results (even if not quite the same way), not so much that we should have backward compatibility. I haven't thought much about the question of whether we need to maintain backward compatibility, though my inclination is that it might not be needed since we're a small group and our projects are at the point that we can adjust relatively easily. I do like the idea of streamlined code, especially if we provide sufficient examples (as we've already said we need) of how to set up the model.
That being said, I did pull this PR and ran it with both my base config/Snakefile and the GW loss config / Snakefile and it ran without error.
Okay. Thanks for clarifying, @janetrbarclay and thanks for your thoughts, @SimonTopp.
I agree with the sentiment of "let's simplify even at the expense of backwards-compatibility" that I think I'm hearing from both of you.
That being the case, I will close this PR and let's go back to the original PR: #148
This PR is a backwards-compatible alternative to #148. I think this could be a better approach. It opens up some flexibility, without sacrificing existing functionality. Specifically it accomplishes this by
trainer
class to a more generictrain
function.train_model
function which I see as a wrapper around thetrain
function that provides some additional structuretrain_model
train_model
function, but they want to use their own custom model and they don't want to code it into thetrain_model
function, they can just pass it in as themodel
argument and bypass theif else
statements for defining and compiling the model.river-dl
codebase.RGCN
andLSTM
) are still accessible via themodel_type
argument like before.I would love your feedback, @janetrbarclay and @SimonTopp. And it would be super helpful if you could pull this branch and see if it indeed works with your existing Snakefiles.