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

[FEM] Elmer: refactor writer #8362

Merged
merged 2 commits into from Feb 6, 2023

Conversation

donovaly
Copy link
Member

@donovaly donovaly commented Feb 5, 2023

  • the writer.py is to large to keep the overview, thus sort out the handling of the different equations (also since more equations will be added)
  • This PR sorts out the electrostatic equation handling is the test balloon if this works well (local tests are all OK)

@donovaly donovaly requested a review from chennes February 5, 2023 17:59
@github-actions github-actions bot added the WB FEM Related to the FEM Workbench label Feb 5, 2023
@chennes
Copy link
Member

chennes commented Feb 5, 2023

There will be a short delay before I can look at this, I spend tomorrow on airplanes. I will look when I am able, but if other reviewers approve don't wait for me :) .

@donovaly
Copy link
Member Author

donovaly commented Feb 5, 2023

There will be a short delay before I can look at this, I spend tomorrow on airplanes. I will look when I am able, but if other reviewers approve don't wait for me :) .

No problem. If you could tell me if I did it right with the underscore naming of the defs this PR should be an easy merge since no code is actually changed, just moved around and the CI tests assure that everything works.

@donovaly
Copy link
Member Author

donovaly commented Feb 5, 2023

Here is an electrostatics test file to check the PR:
Electrostatics3.zip

@freecadci
Copy link

pipeline status for feature branch PR_8362. Pipeline 767982997 was triggered at da13170. All CI branches and pipelines.

@donovaly
Copy link
Member Author

donovaly commented Feb 5, 2023

@chennes , I wish you a good flight back home. Many thanks for all your great work for FreeCAD! I hope we once meet in person (my plan is since a while to organize a full FC conference - one weekend just for FC)

@donovaly donovaly force-pushed the FEM-write-sortout-electrostatics branch from da13170 to b7a02f0 Compare February 5, 2023 20:34
@freecadci
Copy link

pipeline status for feature branch PR_8362. Pipeline 768025713 was triggered at b7a02f0. All CI branches and pipelines.

@luzpaz
Copy link
Contributor

luzpaz commented Feb 5, 2023

JFYI, Some trailing whitespace introduced in this PR: https://github.com/FreeCAD/FreeCAD/actions/runs/4098361865/jobs/7067414783

@donovaly
Copy link
Member Author

donovaly commented Feb 5, 2023

JFYI, Some trailing whitespace introduced in this PR: https://github.com/FreeCAD/FreeCAD/actions/runs/4098361865/jobs/7067414783

Where do you see this? I only got one reported. This is now fixed together with other things noted by Pylint.

donovaly added a commit to donovaly/FreeCAD that referenced this pull request Feb 5, 2023
@freecadci
Copy link

pipeline status for feature branch PR_8362. Pipeline 768038005 was triggered at 9312ee4. All CI branches and pipelines.

@freecadci
Copy link

pipeline status for feature branch PR_8362. Pipeline 768193350 was triggered at 00f4e9c. All CI branches and pipelines.

@donovaly donovaly force-pushed the FEM-write-sortout-electrostatics branch from 00f4e9c to fe8f573 Compare February 6, 2023 05:21
- the writer.py is to large to keep the overview, thus sort out the handling of the different equations (also since more equations will be added)
- This PR sorts out the electrostatic equation handling as first step (more equations will follow once this is merged)
- output only a reasonable number of digits for vacuum permittivity
- output the FC label of potential constraints as comment (helps a lot when having several constraints)
@donovaly donovaly force-pushed the FEM-write-sortout-electrostatics branch from fe8f573 to 76695aa Compare February 6, 2023 05:22
@freecadci
Copy link

pipeline status for feature branch PR_8362. Pipeline 768199011 was triggered at 76695aa. All CI branches and pipelines.

@chennes
Copy link
Member

chennes commented Feb 6, 2023

Side note: we should have a discussion, maybe in a GitHub issue, about the Pylint config file. If you and I are both using it, and the CI runs it, I feel like it could use some fine-tuning so it does not actually encourage worse code.

That said, one way of checking that you have the correct functions underscored or not is to underscore all of them, and then run pylint and see which calls it yells at you about: it should complain about any non-self-prefixed calls to underscored methods. That's useful when refactoring big code where you don't know exactly what needs to be public.

@donovaly
Copy link
Member Author

donovaly commented Feb 6, 2023

then run pylint and see which calls it yells at you about

This is what I already did - I checked the Pylint output of the CI and apply fixed accordingly. I think I caught all underscores now.

In General, since I am no Pythionist, call myself "just a user" ;-), I cannot state about how Pylint is configured.

@donovaly
Copy link
Member Author

donovaly commented Feb 6, 2023

OK, I think this PR is a safe code refactoring. I used it a lot the last day and since no code was actually changed, I need to merge it in order to get other PRs ready.

I will continue the sortout in further PRs.

@donovaly donovaly merged commit e90102b into FreeCAD:master Feb 6, 2023
@donovaly donovaly deleted the FEM-write-sortout-electrostatics branch February 6, 2023 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WB FEM Related to the FEM Workbench
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants