Skip to content

H transport initialisation refactoring#661

Merged
jhdark merged 7 commits into
festim-dev:fenicsxfrom
jhdark:h_transport_initialisation
Dec 5, 2023
Merged

H transport initialisation refactoring#661
jhdark merged 7 commits into
festim-dev:fenicsxfrom
jhdark:h_transport_initialisation

Conversation

@jhdark
Copy link
Copy Markdown
Collaborator

@jhdark jhdark commented Dec 4, 2023

Proposed changes

We habe a minor python bug when multiple instances of the same class are within the same script, and with attributes that default to empty lists, if a value is appended to this list, it becomes the default for subsequent instances like so:

model_1 = F.HydrogenTransportProblem()
model_1.reactions.append(1)

model_2 = F.HydrogenTransportProblem()
print(model_2.reactions)

gives:

[1]

The changes presented fixes this by altering the attribute defaults in HydrogenTransportProblem from

class HydrogenTransportProblem:
    def __init__(self, attribute=[]):
        self.attribute = attribute

to:

class HydrogenTransportProblem:
    def __init__(self, attribute=None):
        self.attribute = attribute

        if self.attribute is None:
            self.attribute = []

and this seems to fix it. Found this in a handy pitfall guide: https://docs.python-guide.org/writing/gotchas/

Types of changes

What types of changes does your code introduce to FESTIM?

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code refactoring
  • Documentation Update (if none of the other choices apply)
  • New tests

Checklist

  • Black formatted
  • Unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (71039f3) 98.87% compared to head (93c92c1) 98.87%.

Additional details and impacted files
@@           Coverage Diff            @@
##           fenicsx     #661   +/-   ##
========================================
  Coverage    98.87%   98.87%           
========================================
  Files           23       23           
  Lines          974      974           
========================================
  Hits           963      963           
  Misses          11       11           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Collaborator

@RemDelaporteMathurin RemDelaporteMathurin left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks for fixing this. It is a common pitfall in python!

Just have a few comments for future proofing 🚀

Comment on lines +103 to +110
self.subdomains = subdomains or []
self.species = species or []
self.reactions = reactions or []
self.temperature = temperature
self.sources = sources
self.boundary_conditions = boundary_conditions
self.sources = sources or []
self.boundary_conditions = boundary_conditions or []
self.settings = settings
self.exports = exports
self.exports = exports or []
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice hack, I didn't know of this way of doing things.
Does it work for all python versions?

Also, we may want to add a comment to say what this does for people like me who don't know this

Comment thread test/test_h_transport_problem.py Outdated
@RemDelaporteMathurin RemDelaporteMathurin mentioned this pull request Dec 5, 2023
10 tasks
jhdark and others added 2 commits December 5, 2023 11:22
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
@jhdark jhdark merged commit d601d06 into festim-dev:fenicsx Dec 5, 2023
@jhdark jhdark deleted the h_transport_initialisation branch December 5, 2023 10:35
@RemDelaporteMathurin RemDelaporteMathurin added the fenicsx Issue that is related to the fenicsx support label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fenicsx Issue that is related to the fenicsx support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants