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

Gridlabd datamodel with common DiTTo reader #349

Open
wants to merge 2 commits into
base: kd/gridlabd-datamodel
Choose a base branch
from

Conversation

daniel-thom
Copy link
Collaborator

This integrates the datamodel that Dheepak created with a proposal for a new common reader.

The main new concept here is this:

  • New class ditto.readers.reader.DiTToReader is responsible for controlling the parsing process.
  • DiTToReader creates a reader interface for the simulation engine specified by the inputs.
  • New class ditto.readers.reader_interface.ReaderInterface specifies the required methods for a reader.
  • The new gridlabd reader implements the reader interface. This is not actually functional. It creates an instance of the gridlabd datamodel from a raw file but does not convert it to a DiTTo model.

I think this design would be better if the Store object was not stored within each DiTTo model instance. I'm OK with keeping it like this.


import enum

class SimulationEngine(enum.Enum):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This may not be required.

"""

@abc.abstractmethod
def read_dataset(self, path, model, **kwargs):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggestions on better terminology are welcome.

@tarekelgindy
Copy link
Contributor

My understanding of this proposal is that we are

  1. Consolidating the parse function so that one parse function applies to all readers. This provides consistency and a single unified place for us to make calls like model.set_names(). I agree that this is a good idea.
  2. Removes the automatic addition of models to the store. This would mean that Line() is called rather than Line(model), and the addition of the line to the Store object would be done separately in the parse function with all the Line, Capacitor etc. objects that are returned from the list_lines(), list_capacitors() etc functions. The purpose would be to remove bi-directional dependencies between Line and Store objects. @kdheepak I think you might have a stronger opinion on this than me - what are your thoughts?

@daniel-thom feel free to correct me if I'm misinterpreting or omitting certain pieces.

@daniel-thom
Copy link
Collaborator Author

daniel-thom commented Aug 21, 2020

From talking with Dheepak I don't think it will be practical to remove storage of the Store in the Line (or other component). What we could do as an intermediate step is remove that attachment in the Line constructor. The main reader could still perform this attachment so that the final behavior is the same. That could make it easier to take the next step in the future.

This still could be bad for two reasons:

  1. It would take some work to make the change.
  2. I'm not positive that it would work. There would not be a back reference to the Store until that list function was complete. Something might need that reference. You guys mentioned that there are two scenarios when converting a component to ditto: 1) information is self-contained and can be directly converted, 2) you have to combine information from multiple components to convert one component. That second case might require access to the model.

Just as a comment, I think the code would be easier to reason about if the Store was only modified in ditto/store.py.

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