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

Make argument names consistent, update docstring, & add units #240

Merged
merged 30 commits into from
Feb 2, 2024

Conversation

FarnazH
Copy link
Member

@FarnazH FarnazH commented Feb 1, 2024

  • Update the docstring of the argument center and specify atomic units
  • Arguments sectors_degree and deg_sectors were used to refer to degree sectors. so they were both changed to d_sectors.
  • sectors_r and radius_sectors were used to refer to radius sectors, so they were both changed to r_sectors. Unit was clarified in docstring.

- Update docstring of argument center and specify atomic units
- Arguments sectors_degree and deg_sectors were used to refer to degree sectors.
  so they were both changed to d_sectors.
- sectors_r and radius_sectors were used to refer to radius sectors,
  so they were both changed to r_sectors. Unit was clarified in docstring.
@marco-2023
Copy link
Collaborator

The changes are good, but now several tests are failing (I will update them).

marco-2023 and others added 2 commits February 2, 2024 01:02
None as defaults is needed for degrees and size
so that an angular grid can be created using any
of the two without defining the other.
@marco-2023
Copy link
Collaborator

@FarnazH I had to use defaults (None) for the degree and size arguments of the AngularGrid __init__. If no default argument is given for degree, then, when initializing from size, it is necessary to provide a degree=None. This was breaking many of the tests. Please feel free to change this (it just was the only way that I could find to solve this issue).

@FarnazH
Copy link
Member Author

FarnazH commented Feb 2, 2024

@FarnazH I had to use defaults (None) for the degree and size arguments of the AngularGrid __init__. If no default argument is given for degree, then, when initializing from size, it is necessary to provide a degree=None. This was breaking many of the tests. Please feel free to change this (it just was the only way that I could find to solve this issue).

@marco-2023, the degree should be set to None by the user if they want to make a grid using the size. That's the API change I proposed in PR #239, which is merged. In my opinion, it does not make sense to set the default values of None for degree and size when at least one of them should be provided. I understand that we want to support two ways of initializing the grid, but the concept of optional arguments is misused. As it is more common to use degree, the user can set it to None, if they choose to use size.

We can talk more about this today, as this is an important API change appearing in other classes as well.

@FarnazH
Copy link
Member Author

FarnazH commented Feb 2, 2024

@Ali-Tehrani and @marco-2023, the notebook failure can be ignored for now. We are changing the API, and just updating the JCP_paper.ipynb that is needed for submitting the paper. Then we can go back and fix the other tutorial notebooks.

@Ali-Tehrani
Copy link
Collaborator

Ali-Tehrani commented Feb 2, 2024

  • The changes made here don't work for python 3.7, so I added the fixes in commit afb9a8e. Also added python 3.7, 3.8 to github actions to make it more evident (I thought I did this before but guess not).
  • Fix the ruff linter complaints with using Union[a, b] to a | b.
  • Fixed the importlib problem with using Python =3.7.

I'm indifferent on either API choices. As long as it's documented, I'm okay with either.

@Ali-Tehrani
Copy link
Collaborator

My bad , I didn't realize running JCP_paper was installing the master branch of grid and so I was constantly getting errors since the changes in the Pull-request weren't made. I think this is good to merge and will fix most of the problems.

@FarnazH FarnazH merged commit c1d558f into master Feb 2, 2024
11 of 12 checks passed
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.

3 participants