Introduce model collection - #523
Open
mabruzzo wants to merge 32 commits into
Open
Conversation
…ing the potential calculation
…laxy object passed by arg
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To be reviewed after #521 and #386 are merged (originally, the plan was to make this not depend on #386, but I changed my mind)
The goal is to introduce the
ModelCollectiontype for tracking different types of Models. The primary example of a model isDiskGalaxy. However, for now, its something of a special case (more on that in a moment).Overview
I've written up a primer about the purpose and motivation for the
ModelCollectionand I've included it in the internal doxygen-based API documentation. A preview of that documentation (prebuilt-by-readthedocs for this PR) can be found.Let me know if you think parts of this discussion are incomplete.
As mentioned in the writeup, we currently track (behind the scenes) different kinds of models inside of a
std::variant(this is simply a type-safe union). I generally think that it would be better to have different types of models inherit from a base class (perhapsBaseModel), but couldn't really do that whileDiskGalaxyremains a special case. I'm happy to make a PR to change this (to follow up PR #524 whereDiskGalaxyis made into a non-special case), but it's probably fine to leave things as they currently stand.DiskGalaxy: a special caseAs of right now,
DiskGalaxyis a bit of a special case that is always constructed (with a fixed configuration). This is totally fine for the way that codebase currently works and doesn't interfere with non-galaxy problems. The followup PR (PR #524) makes it so thatDiskGalaxyis constructed based upon parameters in a parameter file.