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

Materials, Exports, DerivedQuantities, ... should be a subclass of list #494

Closed
4 tasks done
RemDelaporteMathurin opened this issue Jul 1, 2022 · 7 comments · Fixed by #697
Closed
4 tasks done
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@RemDelaporteMathurin
Copy link
Collaborator

RemDelaporteMathurin commented Jul 1, 2022

Users should be able to do

my_materials = festim.Materials()

tungsten = festim.Material(....)

my_materials.append(tungsten)

To do so, the current Materials class:

class Materials:
"""
Args:
materials (list, optional): contains festim.Material objects.
Defaults to [].
"""
def __init__(self, materials=[]):
self.materials = materials

Needs to become:

class Materials(list):

    def __init__(self, *args):
        super().__init__(*args)

Then instead of doing:

my_materials = festim.Materials(......)

for mat in my_materials.materials:
    ...

one would do:

my_materials = festim.Materials(......)

for mat in my_materials:
    ...

Moreover, this:

for mat in self.materials:

would become:

for mat in self:

Progress track:

@RemDelaporteMathurin
Copy link
Collaborator Author

Same for Exports, Traps, DerivedQuantities....?

@RemDelaporteMathurin RemDelaporteMathurin changed the title Materials should have a .append() method Materials should be a subclass of list Oct 10, 2023
@RemDelaporteMathurin RemDelaporteMathurin added enhancement New feature or request good first issue Good for newcomers labels Jan 16, 2024
@KulaginVladimir
Copy link
Collaborator

@RemDelaporteMathurin. Could you please clarify the practical benefit of it a bit more? As I understand, this enhancement will make more like a built-in class, so the users won't need to manually create lists every time and work with them, but rather use documented methods of the FESTIM class (I assume that the docs also will be modified) + some code refactoring. Am I correct?

@RemDelaporteMathurin
Copy link
Collaborator Author

  1. It is super annoying for users to write
my_materials = F.Materials([....])

second_material = my_materials.materials[1]

thanks to this change, this will become much simpler:

my_materials = F.Materials([....])

second_material = my_materials[1]

Same for boundary conditions, traps, derivedquantities, exports, and others.

  1. it will clear a lot of the code base

@KulaginVladimir
Copy link
Collaborator

KulaginVladimir commented Jan 30, 2024

Ok, I can try. How we'll handle it: for all classes at once or iteratively?

@RemDelaporteMathurin
Copy link
Collaborator Author

let's start with Materials

@RemDelaporteMathurin RemDelaporteMathurin changed the title Materials should be a subclass of list Materials, Exports, DerivedQuantities, ... should be a subclass of list` Feb 1, 2024
@RemDelaporteMathurin RemDelaporteMathurin changed the title Materials, Exports, DerivedQuantities, ... should be a subclass of list` Materials, Exports, DerivedQuantities, ... should be a subclass of list Feb 1, 2024
@KulaginVladimir
Copy link
Collaborator

KulaginVladimir commented Feb 1, 2024

@RemDelaporteMathurin, what do you think of TXTExports considering its deprecation?

@RemDelaporteMathurin
Copy link
Collaborator Author

@RemDelaporteMathurin, what do you think of TXTExports considering its deprecation?

I would ignore it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants