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

Simplify tower types #11

Closed
wants to merge 3 commits into from

Conversation

samsrabin
Copy link
Owner

Description of changes

Simplifies child types of TowerSite (NeonSite, Plumber2Site).

Specific notes

Contributors other than yourself, if any: None

CTSM Issues Fixed: None

Are answers expected to change (and if so in what way)? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Does this create a need to change or add documentation? Did you do so? No

Testing performed, if any:

  • All Python unit testing
  • test_sys_run_tower.py

@samsrabin samsrabin self-assigned this Feb 15, 2025
xmlchange,
)
def __init__(self, *args, **kwargs):
super().__init__("NEON", *args, **kwargs)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would indeed be a lot cleaner... Just for my own knowledge, if the args are slightly different between the two, would it still be helpful to describe the arguments and types/definitions here, or is that better suited for the tower_site class with specifications on what applies to neon/plumber listed there?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best for any differences to be described there, yeah, since that's where the differences would actually manifest. Any notes here would need to be kept up-to-date with changes in TowerSite.

@@ -120,9 +120,9 @@ def test_plumber_site(self):
# assert that AR-SLu directories were created during setup
self.assertTrue("AR-SLu" in glob.glob(self._tempdir + "/AR-SLu*")[0])

def test_xmlchange_neon(self):
def test_xmlchange(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also make testing take less time! :)

finidat = "dummy_finidat"

# try to create site
invalid_type = "INVALIDTYPE"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also see this being helpful in the future if anyone wanted to create their own class type.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I was thinking! Although I've been having second thoughts—I'm not sure how useful this invalid type check really is. It'd be easier for someone to make their own new type if they didn't have to add it to the list of accepted types.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that if it's really clear where the type is added, and if that makes it easier to track down what other things need to be done to add a new type, it could be useful. Also, realistically, I might be somewhat surprised if more types are added given the lack of future funding on this project, so maybe this isn't a major concern one way or another right now.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks. I'll get rid of it, then.

  • Get rid of invalid site type check.

@samsrabin
Copy link
Owner Author

Replacing with ESCOMP#2969.

@samsrabin samsrabin closed this Feb 18, 2025
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