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

Add getter and setter for tags. #186

Merged
merged 3 commits into from
Apr 26, 2023
Merged

Conversation

BuczynskiRafal
Copy link
Collaborator

Hi,
I needed to get the tags from the inp file so I added a getter and setter to this section.
For the class "inp" they work fine. For the class "Model" there is a problem reading the file.

@aerispaha can you suggest where could be the problem?

This is the output from test_model_tags (model.tags.dataframe)

Empty DataFrame
Columns: []
Index: [Subcatch, Subcatch, Subcatch, Subcatch]

It looks like it reads the names correctly.

@BuczynskiRafal BuczynskiRafal marked this pull request as ready for review March 23, 2023 11:16
@aerispaha aerispaha self-requested a review April 7, 2023 14:18

model = Model(MODEL_GREEN_AMPT)
actual_output = model.tags
Copy link
Member

Choose a reason for hiding this comment

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

@BuczynskiRafal I think the reason this fails is because we don't actually have a tags property in the swmmio.Model object.

The Model object is a higher level abstraction whose properties bring together multiple related tables in the inp file. For example, the Model.nodes property combines the [JUNCTIONS], [OUTFALLS], and [STORAGE] tables. However, I don't think this higher-level abstraction is necessary in the case of tags. Is there a use-case for this that I'm not thinking of?

That said, instead of adding a tags property to the swmmio.Model object, we could certainly include tags within the Model.nodes section. This way, we when we want fetch all the relevant information about nodes in our models, we can include any tags that may be assigned in the dataframe that is returned by Model.nodes.dataframe.

Does that make sense to you?

Copy link
Collaborator Author

@BuczynskiRafal BuczynskiRafal Apr 8, 2023

Choose a reason for hiding this comment

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

Yes, I agree with you. I left the setter and getter for the inp class.

I'm don't know how to solve tag assignment using the approach you propose i.e. by referencing the key from the "section_headers.yml" file.
The tag section is indexed by the element type and not by its name/id.

[TAGS]
Subcatch  CA-1             CA              
Subcatch  CA-7             CA              
Subcatch  CA-8             CA              
Subcatch  CA-11            CA
Link      P001             CA

I modified the yaml

composite:
  nodes:
    inp_sections: [junctions, outfalls, storage, tags]
    rpt_sections: [Node Depth Summary, Node Flooding Summary, Node Inflow Summary]
    columns: [InvertElev, MaxDepth, InitDepth, SurchargeDepth, PondedArea,
               OutfallType, StageOrTimeseries, TideGate, MaxD, StorageCurve,
               Coefficient, Exponent, Constant, EvapFrac, SuctionHead,
               Conductivity, InitialDeficit, coords, Tag]

output

          InvertElev  MaxDepth  ...                coords  Tag
Link             NaN       NaN  ...                  None   CA
Outlet          0.00       NaN  ...      [(500.0, 500.0)]  NaN
P001            1.10      20.0  ...      [(100.0, 100.0)]  NaN
P005            1.58       NaN  ...      [(200.0, 200.0)]  NaN
P009            3.60      20.0  ...  [(498.395, 215.716)]  NaN
P011            2.89      20.0  ...      [(400.0, 400.0)]  NaN
Subcatch         NaN       NaN  ...                  None   CA
Subcatch         NaN       NaN  ...                  None   CA
Subcatch         NaN       NaN  ...                  None   CA
Subcatch         NaN       NaN  ...                  None   CA

This worked with conduits method

        tags = dataframe_from_inp(inp.path, "TAGS")
        if "Link" in set(tags.index):
            df = df.merge(dataframe_from_inp(inp.path, "Tags"), left_on="Name", right_on="Name", how="left")

output

   Name InletNode  ... SlopeFtPerFt  Tag
0  P001      P001  ...        0.005   CA
1  P005      P005  ...        0.002  NaN
2  P009      P009  ...        0.004  NaN
3  P011      P011  ...        0.002  NaN

Copy link
Member

@aerispaha aerispaha left a comment

Choose a reason for hiding this comment

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

@BuczynskiRafal thanks so much for this contribution. I have a couple minor comments, but I'm going to approve this PR. I'll work on getting the new tags feature released in a couple hours.

@@ -212,6 +213,10 @@ def conduits(self):
df.InletNode = df.InletNode.astype(str)
df.OutletNode = df.OutletNode.astype(str)

tags = dataframe_from_inp(inp.path, "TAGS")
Copy link
Member

Choose a reason for hiding this comment

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

Because you've added tags to the inp object, you can simplify this line to

tags = inp.tags

@@ -212,6 +213,10 @@ def conduits(self):
df.InletNode = df.InletNode.astype(str)
df.OutletNode = df.OutletNode.astype(str)

tags = dataframe_from_inp(inp.path, "TAGS")
if "Link" in set(tags.index):
df = df.merge(dataframe_from_inp(inp.path, "Tags"), left_on="Name", right_on="Name", how="left")
Copy link
Member

Choose a reason for hiding this comment

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

Instead of re-instantiating a tags dataframe, you could reuse the tags dataframe created on line 216, right? Then this line could be simplified to

df = df.merge(tags, left_on="Name", right_on="Name", how="left")

@@ -1314,6 +1322,20 @@ def timeseries(self, df):
"""Set inp.timeseries DataFrame."""
self._timeseries_df = df

@property
def tags(self):
Copy link
Member

Choose a reason for hiding this comment

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

perfect!

@aerispaha
Copy link
Member

This makes progress on #57 by adding coverage for [TAGS]. Thanks again @BuczynskiRafal!

@aerispaha aerispaha merged commit 4084dae into pyswmm:master Apr 26, 2023
@BuczynskiRafal BuczynskiRafal deleted the read_tags branch April 27, 2023 03:48
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