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

Atom borders #897

Merged
merged 2 commits into from
Feb 28, 2025
Merged

Atom borders #897

merged 2 commits into from
Feb 28, 2025

Conversation

pfebrer
Copy link
Contributor

@pfebrer pfebrer commented Feb 28, 2025

Added the possibility to tweak atom borders in the plots with 2D views, using the border_color and border_width keys.
I added the border modification to the example notebook. A poll in Discord has determined that black borders should be the default so this is what I have done here.

I have also taken the opportunity to document the file that plots lines from xarray data, which is quite complex and was undocumented.

):
line_style = {
"color": l_color,
"width": l_width,
"opacity": l_opacity,
"dash": l_dash,
}
marker_style = {"line_width": b_width, "line_color": b_color}
marker_style = {k: v for k, v in marker_style.items() if v is not None}

Check warning

Code scanning / CodeQL

Variable defined multiple times Warning

This assignment to 'marker_style' is unnecessary as it is
redefined
before this value is used.
This assignment to 'marker_style' is unnecessary as it is
redefined
before this value is used.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a CodeQL bug 😅

Copy link

codecov bot commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 86.82%. Comparing base (486a988) to head (04c19d8).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/sisl/viz/plotters/xarray.py 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #897      +/-   ##
==========================================
- Coverage   86.82%   86.82%   -0.01%     
==========================================
  Files         405      405              
  Lines       53308    53314       +6     
==========================================
+ Hits        46286    46290       +4     
- Misses       7022     7024       +2     

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

@zerothi
Copy link
Owner

zerothi commented Feb 28, 2025

Now you'll be the Guinea pig of trying out the towncrier release notes, see in changes/

@pfebrer
Copy link
Contributor Author

pfebrer commented Feb 28, 2025

It is a bit strange because first you have to create the PR to know what is the number, and then you have to create an extra commit to add the file with the changes.

Do you envision that then all the changes are squashed into a single commit? Or there will always be an individual commit for adding the change file?

It complicates a bit the process of submitting a PR 😅

@pfebrer
Copy link
Contributor Author

pfebrer commented Feb 28, 2025

And now all tests are running again just because I added the change file 😅 Although I guess this could be easily fixed

@pfebrer
Copy link
Contributor Author

pfebrer commented Feb 28, 2025

Maybe it would help to be able to write the file before with a name like PR.feat.rst, and then on merge the file could be renamed automatically with the right number.

Or the description of the change could be part of the initial PR comment here in github and picked up automatically when merging. That would be less of a burden for the PR submitter.

@zerothi
Copy link
Owner

zerothi commented Feb 28, 2025

It is a bit strange because first you have to create the PR to know what is the number, and then you have to create an extra commit to add the file with the changes.

Agreed, this also bothers me.
This, or the quite often merge request problem of the CHANGELOG.md, this one will never cause merge-problems.

Do you envision that then all the changes are squashed into a single commit? Or there will always be an individual commit for adding the change file?

Likely that would be great.

It complicates a bit the process of submitting a PR 😅

Agreed... :(

Maybe it would help to be able to write the file before with a name like PR.feat.rst, and then on merge the file could be renamed automatically with the right number.

Or the description of the change could be part of the initial PR comment here in github and picked up automatically when merging. That would be less of a burden for the PR submitter.

I think this would be a good approach. I should look into this.

@zerothi zerothi merged commit b5fc087 into zerothi:main Feb 28, 2025
16 of 17 checks passed
@zerothi
Copy link
Owner

zerothi commented Mar 1, 2025

And now all tests are running again just because I added the change file 😅 Although I guess this could be easily fixed

Ok, I know why that happened.

We do have a check for only changing py files. However, you force-pushed, meaning you created a new commit with changes in py files. If you had done a simple commit, only touching the changes/ folder, then nothing would have happened. ;)

@pfebrer
Copy link
Contributor Author

pfebrer commented Mar 1, 2025

Did I? I think the force push was before the commit that adds the change file. But maybe the tests didn't run, I just saw the CI run and assumed that the tests would run

@zerothi
Copy link
Owner

zerothi commented Mar 1, 2025

Did I? I think the force push was before the commit that adds the change file. But maybe the tests didn't run, I just saw the CI run and assumed that the tests would run

Hmm, yeah, I missed that... I have found the culprit. A GA runs if the paths matches for any of the commits. :(

@pfebrer pfebrer deleted the atom_borders branch March 7, 2025 20:38
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