Skip to content

Conversation

@mbueti
Copy link

@mbueti mbueti commented Feb 28, 2020

No description provided.

mbueti added 24 commits December 3, 2019 15:58
-Interpolation onto depth space is very expensive
-Does not need to happen at every timestep
-Rename Module to ModuleWrapper to avoid confustion
-Allow user to defer module construction to the wrapper
-Allows for easier access to modules
-Check that we're not conflicting with existing model attributes
north/south

-Still need to check for uniqueness
-Still need to enforce that couplers only get two neighbors
-We can now support arbitrary (non-mixed layer) basin configurations
-Still need to validate uniqueness and overloading of couplers
-Coupler can be connected to no more than two modules
-Coupler cannot be connected in the same direction multiple times
@MFJansen
Copy link

MFJansen commented Mar 2, 2020

If I try to run example_twocol_plusSO_model.py I get the following error:
AttributeError: 'ModuleWrapper' object has no attribute 'validate_neighbor_links'
, thrown from add_module in module.py (l.40). I guess this should probably should be "validate_neighbors" instead?

On a separate note: it seems somewhat awkward to me that neighbors have to be added via the neighbor class. Is that class really necessary? Wouldn't it be nicer if we could just point directly to the module wrappers as neighbors? From what I can see the only information that the neighbor class adds is the direction. Couldn't we just solve that by having left_neighbors and right_neighbors in the module_wrapper?

@mbueti
Copy link
Author

mbueti commented Mar 3, 2020 via email

@MFJansen
Copy link

MFJansen commented Mar 3, 2020

I agree that we want to allow multiple right or left neighbors, but can't we just do that with lists?

-Remove neighbor class
-Move more validation into the module_wrapper
-Track explicit left and right neighbor lists
-Short circuit backlinking in a friendlier way
@mbueti
Copy link
Author

mbueti commented Mar 4, 2020

@MFJansen I've poked around a bit, and think the current iteration should be a bit cleaner in functionality. The TwoColumn model has a working example, and you can see the API changes there. I think in this case it makes for a more straightforward setup to pass the neighboring module itself through than the code generated key, especially since we'll need that downstream. Thoughts?

@mbueti
Copy link
Author

mbueti commented Mar 11, 2020

@MFJansen I've pushed up a draft of the simplified neighbor model. I still haven't documented or commented the code, but functionality should all be there. One addition worth noting is

def __getitem__(self, key): return self._modules[key]

Which will let users access modules as model['amoc'], as well as the attr syntax of model.amoc. I think it's fair to leave this, and removing the "dot" access, since it's duplicative and will mostly serve to confuse users and bloat the model a bit. Thoughts?

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.

3 participants