search

EnergySystemsModellingLab / MUSE_2.0

Welcome to the MUSE 2.0 repository
GNU General Public License v3.0
1 stars 0 forks source link

Rename columns in CSV files to use "_id" suffix #102

Closed alexdewar closed 2 months ago

alexdewar commented 2 months ago

There are many places in the CSV files where rows in one file are referred to in another, e.g. commodities defined in commodities.csv are referred to via a "commodity_name" column elsewhere. Agents and regions are the exception: they're referred to with "agent" and "region", respectively.

As "name" is a little ambiguous, I've used an "id" suffix instead. In files that provide a list of these IDs (e.g. agents.csv) the column is just "id" now, which is terser and more in keeping with what databases do.

I've changed the "constraint_name" column to "commodity_constraint_id" because I think that's clearer.

Closes #101.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 80.26%. Comparing base (387fd3b) to head (b1ca894).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #102 +/- ## ======================================= Coverage 80.26% 80.26% ======================================= Files 5 5 Lines 76 76 ======================================= Hits 61 61 Misses 15 15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alexdewar commented 2 months ago

@ahawkes Thanks for the review!

How about we use commodity_constraint and process_investment_constraint with no _name suffix? I agree that having id in the names is liable to create confusion.

Edit: @tsmbland if you have any better ideas that'd be great!

ahawkes commented 2 months ago

OK sounds good - thanks. A

From: Alex Dewar @.> Sent: 03 July 2024 13:16 To: EnergySystemsModellingLab/MUSE_2.0 @.> Cc: Hawkes, Adam D @.>; Mention @.> Subject: Re: [EnergySystemsModellingLab/MUSE_2.0] Rename columns in CSV files to use "_id" suffix (PR #102)

This email from @.*** originates from outside Imperial. Do not click on links and attachments unless you recognise the sender. If you trust the sender, add them to your safe senders listhttps://spam.ic.ac.uk/SpamConsole/Senders.aspx to disable email stamping for this address.

@ahawkeshttps://github.com/ahawkes Thanks for the review!

How about we use commodity_constraint and process_investment_constraint with no _name suffix? I agree that having id in the names is liable to create confusion.

— Reply to this email directly, view it on GitHubhttps://github.com/EnergySystemsModellingLab/MUSE_2.0/pull/102#issuecomment-2205938836, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AC37JLI7PMXZBM2B55S3VFLZKPTRVAVCNFSM6AAAAABKJMBA3OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMBVHEZTQOBTGY. You are receiving this because you were mentioned.Message ID: @.***>

alexdewar commented 2 months ago

@dalonsoa I'm on leave as of tomorrow (returning on Weds), but I'm happy for you to merge this if it meets with your approval!

Depending on what order things get merged in, @Ashmit8583 and @abhinavgupta2004 may have to update their PRs to reflect the new column names. Alternatively, I don't mind holding off on merging this PR until the other bits have been merged and then making the changes here. Whatever seems easiest.

alexdewar commented 2 months ago

Cool, ok. In that case, I'll merge it now.

FYI @Ashmit8583 @abhinavgupta2004 you'll need to update your PRs. Only a few of the columns with changed names should affect you, but obviously your existing code won't work without changes.