Skip to content
This repository has been archived by the owner on Mar 1, 2023. It is now read-only.

Add DGModel outer constructor #1654

Merged
merged 1 commit into from
Oct 29, 2020
Merged

Add DGModel outer constructor #1654

merged 1 commit into from
Oct 29, 2020

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Oct 22, 2020 •
edited
Loading

Description

This PR adds a DGModel outer constructor that accepts a DriverConfiguration.

  • Code follows the style guidelines OR N/A.
  • Unit tests are included OR N/A.
  • Code is exercised in an integration test OR N/A.
  • Documentation has been added/updated OR N/A.

@kpamnany
Copy link
Contributor

kpamnany commented Oct 22, 2020 •
edited
Loading

I see the convenience, but this messes up what I see as the implicit module hierarchy by making DGModel dependent on the definition of DriverConfiguration which is external to and "above" DGModel in the hierarchy. In general, the module dependencies should form a tree, not a graph (here's a somewhat related principle). IMO the added convenience is not worth the cost.

@charleskawczynski
Copy link
Member Author

I see the convenience, but this messes up what I see as the implicit module hierarchy by making DGModel dependent on the definition of DriverConfiguration which is external to and "above" DGModel in the hierarchy. In general, the module dependencies should form a tree, not a graph (here's a somewhat related principle). IMO the added convenience is not worth the cost.

Ahh, very true and I agree. Closing.

kpamnany
kpamnany reviewed Oct 23, 2020
View reviewed changes
Copy link
Contributor

@kpamnany kpamnany left a comment

Choose a reason for hiding this comment

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

LGTM. Suggest adding a docstring to the new constructor for here.

@kpamnany
Copy link
Contributor

Duplicate docstring? But clearly ClimateMachine.DGModel is different than DGMethods.DGModel... I wonder if there's a way around this.

@charleskawczynski
Copy link
Member Author

There's an upstream issue about multiple doc strings, but I think a temporary work-around is to just remove DGModel from the driver config API for now.

@charleskawczynski
Copy link
Member Author

Hmm. Some spurious formatting complaints (I did not modify src/Diagnostics/atmos_les_spectra.jl)

@blallen
Copy link
Contributor

blallen commented Oct 23, 2020

Hmm. Some spurious formatting complaints (I did not modify src/Diagnostics/atmos_les_spectra.jl)

I got that too on #1645

@kpamnany
Copy link
Contributor

Ditto in #1559. I just added a separate commit for misc formatting. 🤷‍♂️

@charleskawczynski
Copy link
Member Author

Rebased.

@charleskawczynski
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Oct 29, 2020
@bors @charleskawczynski
Merge #1654
1fcfe6e 
1654: Add DGModel outer constructor r=charleskawczynski a=charleskawczynski

### Description

This PR adds a `DGModel` outer constructor that accepts a `DriverConfiguration`.



Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 29, 2020

Build failed:

@charleskawczynski
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 29, 2020

Build succeeded:

@bors bors bot merged commit 544bfaa into master Oct 29, 2020
@bors bors bot deleted the ck/DGModel_wrapper branch October 29, 2020 18:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@blallen blallen blallen left review comments

@kpamnany kpamnany kpamnany approved these changes

Assignees
No one assigned
Labels
User Interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@charleskawczynski @kpamnany @blallen