Skip to content

pycopancore reduced to core - #192

Merged
jnnsbrr merged 120 commits into
pik-copan:masterfrom
luaschwarz:master
Feb 17, 2025
Merged

pycopancore reduced to core#192
jnnsbrr merged 120 commits into
pik-copan:masterfrom
luaschwarz:master

Conversation

@jnnsbrr

@jnnsbrr jnnsbrr commented Jul 2, 2024

Copy link
Copy Markdown
Collaborator

This PR includes 2 major parts:

General changes to pycopancore

  • replaced relative imports with absolute ones for better overview (https://peps.python.org/pep-0008/#imports)
  • includes some commits also of @mensch72 that did not make it into the master but in the the fork and couple_lpjml branch
  • debugging of both previous bullet points
  • model_logic was made more flexible for missing processes, process_taxa
  • I did not change the functionality of copan:CORE, just made it more flexible for alternative usages

pycopancore split into additional repositories ...

pycopanmodels

  • all components that are not the core (base, etc.)
  • all models
  • all studies

pycopanlpjml new

  • lpjml component
  • relies on pycoupler and pycopancore
  • simplified syntax, structure compared to pycopancore

inseeds new

  • inseeds components (farmer_management, base)
  • models (farmer_management)
  • simplified syntax, structure compared to pycopancore

mensch72 and others added 30 commits November 18, 2021 21:14

@mensch72 mensch72 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so far all looks fine except a few minor comments

@mensch72

mensch72 commented Jan 19, 2025 via email

Copy link
Copy Markdown
Contributor

@zugnachpankow

Copy link
Copy Markdown
Contributor

@jnnsbrr are there any changes that should be double-checked or looked at in particular? i have not worked with core in a while so i am not sure what is most relevant. at a glimpse all seems fine.

jnnsbrr and others added 2 commits January 24, 2025 10:32
@jnnsbrr

jnnsbrr commented Jan 24, 2025

Copy link
Copy Markdown
Collaborator Author

@jnnsbrr are there any changes that should be double-checked or looked at in particular? i have not worked with core in a while so i am not sure what is most relevant. at a glimpse all seems fine.

Do you remember if in the beginning when you and Luana started to work on the coupling what happened here?

I guess one review (@mensch72) should be fine here if you have not worked with it for a while. We also should really proceed here ... 😬

@zugnachpankow

Copy link
Copy Markdown
Contributor

Regarding the generic imitation component and the confusion it created

I checked and do not have these changes in my fork for nexploit nor the group entity merge. I believe that Leander might have included them, see more below. I am not sure if they introduced any breaking changes, so I went through the commits by @mensch72 before the first commit of Luana and tried to summarize.

  • 672c53b: mainly changes to the generic imitation component and its testing that should be irrelevant (exclude), but also some changes to the data model and the base implementations, should be checked and included in case
  • 86fa736: only changes to generic imitation component, should be irrelevant and could be excluded
  • 262f92c: change in gitignore, can probably be included
  • 31fec4b: another change in .gitignore, can probably be included
  • b7fdb57: deletion of build docs, why? this might conflict with changes that i made to the documentation with the group entity
  • f251d8e: includes a change to the docs/makefile (include?) and one to generic imitation component implementation (exclude)
  • e446a7e: some things related to a config regarding imports, should be obsolete with absolute imports introduced here; the rest relates to general imitation component, unimportant
  • 0d0559a: only related to imitation component, can be excluded
  • dafc540: adds a profiler util, might be useful, include?
  • b0547bf: only related to imitation component, can be excluded
  • cb86b08: mixed, includes some changes to data_model and docs that might be included in the merge, the rest is imitation
  • 6cc2423: @jnnsbrr can you have a look at this? i dont get whats going on here, this is very confusing. i think here is where things went wrong. there is an open merge request Issue 152 #156 which seems to be the one where leander kept working on the imitation component. apparently he used the changes up to this point and probably had them on his branch or fork or whatever we then used to start with the coupling on his machine.

I hope this overview helps a bit. Maybe @mensch72 can have a look and see if they are relevant? Unfortunately, it seems that in some of the commits changes that might be relevant to this merge request overlap with some that are not relevant for this merge (i.e. changes that only effect the implementation of the generic imitation component, its testing and study).
Regarding the reason for this overlap: I have started working with copan:CORE only in 2022 so I cannot say for sure what happened in Dec 2021, i think it is most plausible that Leander had some changes that should have been included with #156 on his machine which had not happened by the point of our hackathon and so we carried them over. We could consider rebasing #156? @jnnsbrr what do you think is the best way to continue?

@jnnsbrr

jnnsbrr commented Jan 28, 2025

Copy link
Copy Markdown
Collaborator Author

I removed everything I am aware of the unfinished generic imitations work. Two unsolved conversations pending. @mensch72 can you comment on that? I guess than its ready to merge.

Thanks a lot @zugnachpankow and @mensch72 already!

@mensch72 mensch72 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

see remaining unresolved conversations for what still needs to be done.

Comment thread pycopancore/model_components/base/implementation/world.py
Comment thread pycopancore/model_components/base/interface.py Outdated
Comment thread studies/_testing/generic_imitation_like_example1.py
Comment thread pycopancore/model_components/base/implementation/world.py
Comment thread pycopancore/data_model/master_data_model/dimensions_and_units.py
Comment thread pycopancore/data_model/master_data_model/dimensions_and_units.py Outdated
Comment thread pycopancore/data_model/master_data_model/environment.py Outdated
Comment thread pycopancore/model_components/base/interface.py Outdated
@mensch72

mensch72 commented Jan 29, 2025 via email

Copy link
Copy Markdown
Contributor

@jnnsbrr

jnnsbrr commented Jan 29, 2025

Copy link
Copy Markdown
Collaborator Author

because someone added a relation from socialsystem to culture, which is a convincing addition, that is however in conflict with the relation from world to culture because there is also a relation from socialsystem to world, and if we were to keep both, it could result that socialsystem.culture != socialsystem.world.culture. that's why we should remove the direct relation world.culture and its inverse, culture.worlds. Am 29. Januar 2025 09:03:03 MEZ schrieb Jannes Breier @.>:

@jnnsbrr commented on this pull request. > @@ -115,20 +118,6 @@ def culture(self, c): c._worlds.add(self) self._culture = c - @Property Why should we remove it? -- Reply to this email directly or view it on GitHub: #192 (comment) You are receiving this because you were mentioned. Message ID: @.
>

Got it, thanks for clarification.
I am just following your instructions here, but this is not tested at all. We need heavy unit testing here ...

remove culture from world since its now part of a social_system and not directly of a world any longer
@jnnsbrr
jnnsbrr dismissed mensch72’s stale review January 29, 2025 10:59

Done already, I do not understand githubs response here. Cannot solve something that I have already solved.

@jnnsbrr
jnnsbrr removed the request for review from hannah-pra January 29, 2025 11:00
@jnnsbrr

jnnsbrr commented Jan 29, 2025

Copy link
Copy Markdown
Collaborator Author

@zugnachpankow (review pending - can be short - you already it basically) and @mensch72 if you would both give your OK again then I guess some of you could hit merge.

@jnnsbrr
jnnsbrr marked this pull request as ready for review January 29, 2025 11:01
@mensch72

mensch72 commented Jan 29, 2025 via email

Copy link
Copy Markdown
Contributor

@jnnsbrr

jnnsbrr commented Jan 29, 2025

Copy link
Copy Markdown
Collaborator Author

oh i don't want to take that responsibility... Am 29. Januar 2025 12:02:12 MEZ schrieb Jannes Breier @.>:

@zugnachpankow (review pending - can be short - you already it basically) and @mensch72 if you would both give your OK again then I guess some of you could hit merge. -- Reply to this email directly or view it on GitHub: #192 (comment) You are receiving this because you were mentioned. Message ID: @.
>

@zugnachpankow hit it!

@luaschwarz

luaschwarz commented Jan 29, 2025 via email

Copy link
Copy Markdown
Contributor

@zugnachpankow zugnachpankow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry, cant hit it yet: the utils were already part of master, only the porfiling util was new; e.g. the seed setter is used in some studies afaik; why where they removed? did you mean to just remove the profiler?

Comment thread pycopancore/util/__init__.py
Comment thread pycopancore/util/functions.py
Comment thread pycopancore/util/seeding.py

@zugnachpankow zugnachpankow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks all good now, let's go!

@jnnsbrr
jnnsbrr merged commit 036e452 into pik-copan:master Feb 17, 2025
@zugnachpankow

Copy link
Copy Markdown
Contributor

Congrats 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement lpjml stuff concerning the coupling to LPJmL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants