Skip to content

Review and modernization of the package#280

Closed
kshitij-maths wants to merge 0 commit intomathLab:masterfrom
kshitij-maths:master
Closed

Review and modernization of the package#280
kshitij-maths wants to merge 0 commit intomathLab:masterfrom
kshitij-maths:master

Conversation

@kshitij-maths
Copy link
Member

@kshitij-maths kshitij-maths commented Nov 17, 2025

This PR introduces several modernization improvements to the package, including:

  • Migrated the build system from setup.py to a modern pyproject.toml–based configuration.
  • Updated the package structure and metadata in accordance with current Python packaging standards
  • Revised and refactored supporting files impacted by the migration.
  • Updated tutorials and documentation to ensure compatibility with the current version and the new build system.
  • Performed a general review of the package to align it with modern best practices.

@ndem0
Copy link
Member

ndem0 commented Nov 17, 2025

Thanks @kshitij-maths can you check why all the tests are failing? We should fix them to merge the changes!

@kshitij-maths
Copy link
Member Author

@ndem0 checking.

@kshitij-maths
Copy link
Member Author

Please approve the current workflow. Let's see if it works.

@kshitij-maths
Copy link
Member Author

kshitij-maths commented Nov 19, 2025

@ndem0 The macOS PR test is consistently failing because s-weigand/setup-conda does not install Conda correctly on newer macOS images; all other OSs pass. Attempts to use an older macOS version (macos-12) also cause failures on the other OSs.

@ndem0
Copy link
Member

ndem0 commented Nov 19, 2025

Ok thanks! Yes the action is not working for MacOS, they also have a warning on the main page https://github.com/s-weigand/setup-conda

I would change the action so! I tried to quick search on google and there is the official action developed by miniconda developers https://github.com/conda-incubator/setup-miniconda

Also please try to fix the Codacy issues, there are 64 new issues https://app.codacy.com/gh/mathLab/PyGeM/pull-requests/280/issues

@kshitij-maths
Copy link
Member Author

I would change the action so!

Okay, thanks!

Also please try to fix the Codacy issues, there are 64 new issues https://app.codacy.com/gh/mathLab/PyGeM/pull-requests/280/issues

Okay.

Copy link
Member

@ndem0 ndem0 left a comment

Choose a reason for hiding this comment

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

A huge amount of file are changed because of small formatting (double quotes instead signle one for example). Also, a lot of new file have been added, but I don't think are necessary (they should be the output of some tests).
Additionally, please try to keep few lines of changes for PR, around 100 is completely fine (here we have 6000 lines for 35 commits, definetely to large).

Please remove unnecessary file.
Please restore to the previous version all the file contined in pygem, tests and tutorials folders.

README.md Outdated
Comment on lines 251 to 410
Copy link
Member

Choose a reason for hiding this comment

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

Why? Please revert

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need to revert the complete README to its original version. We've simply reorganized it for better clarity. If you look at the rendered file rather than the raw code, you'll see that the content is essentially the same, perhaps more polished and structured. The overwhelming number of added and deleted lines you see here is likely because I rewrote the file completely, rather than making changes line by line.

@kshitij-maths kshitij-maths marked this pull request as draft November 27, 2025 11:09
@kshitij-maths kshitij-maths requested a review from ndem0 November 27, 2025 11:09
@kshitij-maths kshitij-maths marked this pull request as ready for review November 27, 2025 15:49
pyproject.toml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

add vtk

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

Comments