-
Notifications
You must be signed in to change notification settings - Fork 24
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
Mesh1D vertices to accept list #665
Mesh1D vertices to accept list #665
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## fenicsx #665 +/- ##
===========================================
+ Coverage 98.87% 99.07% +0.19%
===========================================
Files 23 24 +1
Lines 974 1076 +102
===========================================
+ Hits 963 1066 +103
+ Misses 11 10 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small changes to make
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can update the documentation too
Co-authored-by: James Dark <[email protected]>
Thanks Jonathon for your first contribution to festim !!! Hopefully the first of many! Any other changes from you @RemDelaporteMathurin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @JonathanGSDUFOUR! Congrats for this first contribution! 🎉
A couple of comments on my side.
Also the coverage doesn't seem to be uploaded to Codecov.
@jhdark we may have to update the uploader in the github actions workflow.
Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>
@JonathanGSDUFOUR here's a tip to format your scripts. Install black
Then run:
This will format all the files in the current directory. Alternatively you can also install the black extension in Visual Studio Code (if you're using it) |
@JonathanGSDUFOUR right I don't know why the linting is not passing... Maybe different versions of black? the workflow runs with black 23.12.0, what is your black version? |
When I managed to use black it was through pip install black in terminal, but it uses the 19.10b0 version, and when trying to use VSCode extension it doesn't seem to work (the extension is on version 23.10.1) |
This is a very old version of black (2019!!) Can you try to upgrade it with
|
Co-authored-by: Rémi Delaporte-Mathurin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Thanks @JonathanGSDUFOUR and congrats on your first contribution! 💯
@jhdark feel free to merge this
Proposed changes
Added a setter for the vertices in Mesh_1D so that a list in input is converted to ndarray, the correct format.
There is also an added test to check if the format of the vertices is still good.
Types of changes
What types of changes does your code introduce to FESTIM?
Checklist
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...