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

Added Components #33

Merged
merged 5 commits into from
Aug 17, 2023
Merged

Added Components #33

merged 5 commits into from
Aug 17, 2023

Conversation

Aerosmite
Copy link
Contributor

Added simple component class and integrated it to Simulation.

Added Component class and integrated it to Simulation
@arvedes
Copy link
Member

arvedes commented Jul 10, 2023

Thank you for this nice contribution! Can you add a few tests for the new features to pyelmer/tests/test_elmer.py?

added Name in Component class to avoid warning
@Aerosmite
Copy link
Contributor Author

Unfortunately I can't share mine as it is a company project. You can try to generate one of the many example of magnetic simulation in elmer test examples, which often use components.

Also made a small mistake in the Component class, you need to have a name attribute otherwise you get a warning:

def get_data(self):
    """Generate dictionary with data for sif-file."""
    d = {
        "Name": self.name,
        f"Master Bodies({len(self.geo_ids)})": " ".join([str(x) for x in self.geo_ids] )
    }
    d.update(self.data)
    return d

@arvedes
Copy link
Member

arvedes commented Jul 14, 2023

Thanks for the fix! I don't know if we're talking about the same thing. I meant the pyelmer tests, not a complete Elmer simulation. The tests are run with pytest and check the main classes and functions.

It would be nice to have a test for the components, too, e.g. a test_component function similar to test_body and test_duplicate_body.

@arvedes
Copy link
Member

arvedes commented Aug 16, 2023

I just added the tests. I don't have any experience with Components yet, but in the Elmer models manual, it seems to me as if the parameter Master Bodies in the sif file is supposed to refer to a Body also defined in the sif. The way it's implemented now it is supposed to refer to a part of the geometry. Can you clarify that for me?

@Aerosmite
Copy link
Contributor Author

I don’t have access to my computer currently so I can’t be 100% sure, but according to the Elmer doc and because my geometry ids are the same than my body ids in my simulations I think you’re right. Sorry I didn’t see your previous message.

Thanks a lot for your tool anyway! At some point I would have been bored and coded the same thing than you so it really helped me save time!

@arvedes
Copy link
Member

arvedes commented Aug 17, 2023

You're welcome! I updated it so that the component class now has the parameters master_bodies and master_boundaries instead of geo_ids.

What do you think, is it ok like that? Can I merge it?

@Aerosmite
Copy link
Contributor Author

I can’t test it now but it looks perfect!

@arvedes arvedes merged commit 56cfe2d into nemocrys:master Aug 17, 2023
1 check failed
@arvedes
Copy link
Member

arvedes commented Aug 17, 2023

Ok, I merged it and created a new release. Tanks again!

@arvedes arvedes mentioned this pull request Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants