search

calculix / ccx2paraview

CalculiX to Paraview converter (frd to vtk/vtu). Makes possible to view and postprocess CalculiX analysis results in Paraview. Generates Mises and Principal components for stress and strain tensors.
GNU General Public License v3.0
84 stars 18 forks source link

A suggestion for edits necessary to make a python module #10

Closed Krande closed 4 years ago

Krande commented 4 years ago

So I had to rename the "src" folder to be able to make a sensible python module out of it, and I added just a "stub" lining out some of the necessary steps in an azure pipeline file in "ci/azure-pipeline.yml".

Let me know what works and what does not and I will try to meet you half way :)

Best Regards Kristoffer

imirzov commented 4 years ago

Well, Kristoffer,

  1. You've told me about pip and conda packaging. How Asure relates to it? Please, do not mix everything into one pull request. Solve tasks consequently.

  2. You're pulling 3 commits with the same description: "suggested edits necessary to make a python module". How can I distinguish what's done in each commit?

  3. Your branch has conflicts. Please, resolve.

  4. We've agreed that you'll follow my folders structure.

  5. What have you done with files PVDWriter.py, VTKWriter.py, VTUWriter.py? 56, 338, 446 editions - why?

  6. Editions like variable renaming and adding spaces are senseless.

Please, try again.

Krande commented 4 years ago

Hi!

apologies for the messy commit history. I should have waited with the original pull request, but in my eagerness I created it too early and found a couple of obvious mistakes right after (and then found some more after the last commit).

Now to your comments.

  1. Azure Devops would be the mechanism for creating the pypi package automatically on pushes to the master branch. This is a common continuous integration pipeline to automate testing and deployment of not only python packages but for just about any compiled\uncompiled code as well. It is really powerful, and I feel it would be a mistake not using it (or a similar mechanism) for creating the python package!

  2. Yes, that was a series of embarrassing mistakes on my part. Please accept my apologies :) All 3 commits are supposed to cover the same suggestion. In my opinion all changes were intended necessary for the pypi package. In hindsight I probably should have kept the edits regarding the functionality to explicitly set the ouput folder for the result files out of this pull request (the file_name -> file_output stuff).

  3. It might be my setup, but for some reason the tests seem to work on my end. Any particular area that seems to be failing?

  4. Yes, that was a mistake on my part prior to creating the pull request. I should have remembered the reason why I opted for the "/src/ccx2paraview" structure in the first place. To make a python package you need to use the package name as the "top folder" of your package (at least that is what I gathered from reading various sources online this is a good example). Meaning just "src" would result in the package being named "src" after you do a pip install ccx2paraview (which naturally would not be ideal). I see that there are some "hacks" to circumvent this using mlink and stuff like that, but I feel that would be going down the wrong path.

  5. Hm, I don't believe I did more than moving the files from src to ccx2paraview. It might be that my IDE subsequently did something to the line formatting somewhere (or something like that) for git to register it as line edits and not a simple file rename (like what was done for clean.py). I've seen this previously when working on files originally created on linux (I am on windows). Anyways, if you look at the git file comparisons, all the lines of code appears to be the same.

  6. I completely agree. However, some renames were necessary, such as the relative file imports ("from src" only works in the git repo, not as a python package.) that ensures the python package to work. But other then that I do not think I have renamed any variable names just for the sake of renaming.

Perhaps we should take a step back and consider our intended use cases for the repo. If the code is primarily intended to be used from the repo itself then I understand the reluctance to add more layers of complexity to the folder structure (which is something that unfortunately comes with setting up a python package deployment template).

So if you rather prefer keeping the folder structure as is, and keep the repository streamlined for its current intended purpose I would understand and will instead find another way to solve my FEM use case.

Anyways, thanks for getting back to me, and let me know if you are still interested in another pull request (which unfortunately still would require moving away from "src" to either "src/ccx2paraview" or "ccx2paraview" as the containing folder for the package).

Best Regards Kristoffer

imirzov commented 4 years ago

Dear Kristoffer, Thank you and I'm sorry. I wouldn't like to change folder structure and increase complexity.