search

QSD-Group / QSDsan

Quantitative Sustainable Design (QSD) of sanitation and resource recovery systems.
https://qsdsan.com
Other
30 stars 14 forks source link

Merge updates in conventional wastewater configurations #103

Closed yalinli2 closed 1 month ago

yalinli2 commented 1 year ago

@RaiSaumitra I'm making this a draft PR so we can discuss any needed updates, some of my changes could be style-related for easier understanding/improve in speed

yalinli2 commented 1 year ago

@RaiSaumitra I took a quick look and added notes (I think you've started reviewing them so that's great). When @joyxyz1994 is back we should ask her to review before merging into the main branch.

The tests actually passed but something happened with Codecov (this is to check how much code is covered by the tests), not exactly sure why but maybe it's because it's a draft PR not an actual PR? I'll figure this out later.

Thanks!

codecov[bot] commented 1 year ago

Codecov Report

Attention: Patch coverage is 29.18149% with 2189 lines in your changes missing coverage. Please review.

Project coverage is 66.97%. Comparing base (b973f6c) to head (0800ffd). Report is 14 commits behind head on main.

:exclamation: Current head 0800ffd differs from pull request most recent head 3db40fe

Please upload reports for the commit 3db40fe to get more accurate results.

Files Patch % Lines
qsdsan/sanunits/_junction_copy.py 11.01% 622 Missing :warning:
qsdsan/sanunits/_junction.py 2.76% 493 Missing :warning:
qsdsan/sanunits/_clarifier.py 43.29% 305 Missing and 12 partials :warning:
qsdsan/sanunits/_sludge_treatment.py 53.99% 233 Missing and 9 partials :warning:
qsdsan/sanunits/_membrane_gas_extraction.py 25.00% 162 Missing :warning:
qsdsan/sanunits/_activated_sludge_process.py 33.48% 145 Missing :warning:
qsdsan/processes/_adm1_p_extension.py 53.25% 115 Missing :warning:
qsdsan/utils/wwt_design.py 15.23% 89 Missing :warning:
qsdsan/_waste_stream.py 20.00% 3 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #103 +/- ## ========================================== - Coverage 74.18% 66.97% -7.22% ========================================== Files 91 95 +4 Lines 16054 19104 +3050 Branches 1686 1953 +267 ========================================== + Hits 11910 12794 +884 - Misses 3689 5837 +2148 - Partials 455 473 +18 ``` | [Flag](https://app.codecov.io/gh/QSD-Group/QSDsan/pull/103/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=QSD-Group) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/QSD-Group/QSDsan/pull/103/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=QSD-Group) | `66.97% <29.18%> (-7.22%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=QSD-Group#carryforward-flags-in-the-pull-request-comment) to find out more.

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

yalinli2 commented 1 year ago

@RaiSaumitra I resolved conflicts between metro and main, fixed some minor bugs in metro, and merged in main (see cb8a746)

I'll do this periodically in the future as I work on the BSM2 config, let me know if I messed anything on your system, thanks!

ps, the tests are failing due to some other issues and I'm trying to get those fixed

yalinli2 commented 1 year ago

btw on the degradability and organic property of components, we should be consistent with the default (qsdsan/data/_components.tsv): image

but @RaiSaumitra I think you set things differently (metroASM2d.py) image

also, you don't have to use Component.from_chemical if you provide search_ID, i.e., you can just 

     CO2 = qs.Component('S_CO2', search_ID='CO2', particle_size='Dissolved gas', degradability='Undegradable', organic=False)
yalinli2 commented 1 month ago

@joyxyz1994 @RaiSaumitra I think the recent merge related to bsm2 pretty much included all the changes here? The metro branch of EXPOsan is quite outdated now, I'm not sure if there's another repo for the systems for the metro paper?

RaiSaumitra commented 1 month ago

Hi Yalin, yes, the metro branch is now outdated. I am using the metro_dev branch. Are you planning to delete metro?

Thanks, Saumitra

On Wed, Oct 16, 2024 at 10:23 AM Yalin @.***> wrote:

@joyxyz1994 https://github.com/joyxyz1994 @RaiSaumitra https://github.com/RaiSaumitra I think the recent merge related to bsm2 pretty much included all the changes here? The metro branch of EXPOsan is quite outdated now, I'm not sure if there's another repo for the systems for the metro paper?

— Reply to this email directly, view it on GitHub https://github.com/QSD-Group/QSDsan/pull/103#issuecomment-2417160591, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUJZKESHL5NYDUDJFYKOVZ3Z32AG3AVCNFSM6AAAAAA3G3KLDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXGE3DANJZGE . You are receiving this because you were mentioned.Message ID: @.***>

yalinli2 commented 1 month ago

Oh if you are using metro_dev then I'll just remove metro, I don't think I've done anything particular with it... But I don't see metro_dev on GH, did you push it?

From: RaiSaumitra @.> Sent: Wednesday, October 16, 2024 11:26 AM To: QSD-Group/QSDsan @.> Cc: Yalin @.>; Author @.> Subject: Re: [QSD-Group/QSDsan] Merge updates in conventional wastewater configurations (PR #103)

Hi Yalin, yes, the metro branch is now outdated. I am using the metro_dev branch. Are you planning to delete metro?

Thanks, Saumitra

On Wed, Oct 16, 2024 at 10:23 AM Yalin @.***> wrote:

@joyxyz1994 https://github.com/joyxyz1994 @RaiSaumitra https://github.com/RaiSaumitra I think the recent merge related to bsm2 pretty much included all the changes here? The metro branch of EXPOsan is quite outdated now, I'm not sure if there's another repo for the systems for the metro paper?

— Reply to this email directly, view it on GitHub https://github.com/QSD-Group/QSDsan/pull/103#issuecomment-2417160591, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUJZKESHL5NYDUDJFYKOVZ3Z32AG3AVCNFSM6AAAAAA3G3KLDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXGE3DANJZGE . You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHubhttps://github.com/QSD-Group/QSDsan/pull/103#issuecomment-2417168259, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ALV5VLL7VQNNK76XEGGIGQLZ32AQRAVCNFSM6AAAAAA3G3KLDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXGE3DQMRVHE. You are receiving this because you authored the thread.

RaiSaumitra commented 1 month ago

Hi Yalin,

I'm sorry I got the name wrong. It is 'dev_metro'. I have pushed it so you should see it.

Thanks, Saumitra

On Wed, Oct 16, 2024 at 10:31 AM Yalin @.***> wrote:

Oh if you are using metro_dev then I'll just remove metro, I don't think I've done anything particular with it... But I don't see metro_dev on GH, did you push it?

From: RaiSaumitra @.> Sent: Wednesday, October 16, 2024 11:26 AM To: QSD-Group/QSDsan @.> Cc: Yalin @.>; Author @.> Subject: Re: [QSD-Group/QSDsan] Merge updates in conventional wastewater configurations (PR #103)

Hi Yalin, yes, the metro branch is now outdated. I am using the metro_dev branch. Are you planning to delete metro?

Thanks, Saumitra

On Wed, Oct 16, 2024 at 10:23 AM Yalin @.***> wrote:

@joyxyz1994 https://github.com/joyxyz1994 @RaiSaumitra https://github.com/RaiSaumitra I think the recent merge related to bsm2 pretty much included all the changes here? The metro branch of EXPOsan is quite outdated now, I'm not sure if there's another repo for the systems for the metro paper?

— Reply to this email directly, view it on GitHub https://github.com/QSD-Group/QSDsan/pull/103#issuecomment-2417160591, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AUJZKESHL5NYDUDJFYKOVZ3Z32AG3AVCNFSM6AAAAAA3G3KLDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXGE3DANJZGE>

. You are receiving this because you were mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub< https://github.com/QSD-Group/QSDsan/pull/103#issuecomment-2417168259>, or unsubscribe< https://github.com/notifications/unsubscribe-auth/ALV5VLL7VQNNK76XEGGIGQLZ32AQRAVCNFSM6AAAAAA3G3KLDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXGE3DQMRVHE>.

You are receiving this because you authored the thread.

— Reply to this email directly, view it on GitHub https://github.com/QSD-Group/QSDsan/pull/103#issuecomment-2417185678, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUJZKESKTL6VFYZY4OK5LNDZ32BGFAVCNFSM6AAAAAA3G3KLDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXGE4DKNRXHA . You are receiving this because you were mentioned.Message ID: @.***>

yalinli2 commented 1 month ago

Oh the branch is on QSDsan? I was talking about EXPOsan on the system configuration of the systems in metro.

Can you do the following so we'll be more organized?

  1. See if there's anything on the QSDsan metro branch that you'd like to keep.
  2. See if there's anything on the EXPOsan metro branch that you'd like to keep.
  3. Push the system configuration you are currently using to a new EXPOsan branch, maybe dev_metro for consistency, thanks!
RaiSaumitra commented 1 month ago

Hi Yalin,

I do not think I need anything from the metro branch on either QSDsan and EXPOsan. And based on your suggestion, I have published a dev_metro branch on EXPOsan as well. Let me know if you suggest any more edits.

Thanks, Saumitra

On Wed, Oct 16, 2024 at 10:46 AM Yalin @.***> wrote:

Oh the branch is on QSDsan? I was talking about EXPOsan on the system configuration of the systems in metro.

Can you do the following so we'll be more organized?

  1. See if there's anything on the QSDsan metro branch that you'd like to keep.
  2. See if there's anything on the EXPOsan metro branch that you'd like to keep.
  3. Push the system configuration you are currently using to a new EXPOsan branch, maybe dev_metro for consistency, thanks!

— Reply to this email directly, view it on GitHub https://github.com/QSD-Group/QSDsan/pull/103#issuecomment-2417225360, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUJZKEVHURIQ3OQIRUXOERTZ32C5DAVCNFSM6AAAAAA3G3KLDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJXGIZDKMZWGA . You are receiving this because you were mentioned.Message ID: @.***>

yalinli2 commented 1 month ago

Oh cool! I'll archive/remove those branches then, I'll close this issue and wait till it's ready to be merged in, thanks!

yalinli2 commented 1 month ago

Branches archived/removed!

Oh cool! I'll archive/remove those branches then, I'll close this issue and wait till it's ready to be merged in, thanks!

RaiSaumitra commented 1 month ago

Thanks Yalin!

On Wed, Oct 16, 2024 at 8:19 PM Yalin @.***> wrote:

Branches archived/removed!

Oh cool! I'll archive/remove those branches then, I'll close this issue and wait till it's ready to be merged in, thanks!

— Reply to this email directly, view it on GitHub https://github.com/QSD-Group/QSDsan/pull/103#issuecomment-2418297491, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUJZKESTR4WBHYJVYFQHHV3Z34GC7AVCNFSM6AAAAAA3G3KLDGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMJYGI4TONBZGE . You are receiving this because you were mentioned.Message ID: @.***>