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

nfd_solver #2

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Conversation

Sam-Bouten
Copy link
Contributor

include natural force density solver module, and example scripts.

@juney-lee
Copy link
Contributor

@Sam-Bouten Hey Sam, looks like some of the checks were not successful. Do you have a linter to tidy up the code?

Copy link
Collaborator

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

have not looked at the functionality itself. just some general comments...

  • make sure to use an editable install of compas_fd during development and update your solver imports in the sample scripts accorindly
  • many of the output files seem unnecessary. in any case they should be removed from source control
  • documentation!
  • marking the top level package as _numpy should be sufficient to mark the entire set of solvers as numpy based. just make sure to update the public api functions accordingly
  • since completely numpy based you can use type annotations without restrictions. i would encourage you to do so because very helpful during use and further development

src/nfd_solver/nfd/__init__.py Outdated Show resolved Hide resolved
src/nfd_solver/nfd/math_utilities.py Outdated Show resolved Hide resolved
src/nfd_solver/nfd/math_utilities.py Outdated Show resolved Hide resolved
src/nfd_solver/nfd/matrices.py Outdated Show resolved Hide resolved
src/nfd_solver/nfd/matrices.py Outdated Show resolved Hide resolved
src/nfd_solver/scripts/01_hypar.py Outdated Show resolved Hide resolved
src/nfd_solver/scripts/02_hypar_opening.py Outdated Show resolved Hide resolved
src/nfd_solver/scripts/03_hypar_quads.py Outdated Show resolved Hide resolved
src/nfd_solver/scripts/01_hypar.py Outdated Show resolved Hide resolved
src/nfd_solver/scripts/01_hypar.py Outdated Show resolved Hide resolved
@Sam-Bouten
Copy link
Contributor Author

Some thoughts for discussion:

  • Vectorisation

Currently there are global python for-loops in the algorithm:
all NaturalFaces and NaturalEdges are instantiated, and the StiffnessMatrixAssembler
calls each of these instances to provide their force density contributions to the matrix.

Another way would be to have the NaturalFace and NaturalEdge as flyweights; changing their instance methods
into static and calling them on global numpy arrays. This would mean 3d-arrays and more bookkeeping..
But it would avoid the global for-loop.

  • Anchored vertices

Currently the mesh vertex attribute 'is_anchor' is hardcoded as a flag to detect fixed vertices.
This must be updated, but unsure where to elegantly provide the fixed vertices input...

  • math_utilities

Should these functions reside in a more general location?
A few overlap in functionality with compas utilities, but are more specific/optimized.

Copy link
Collaborator

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

  • example scripts should not be included in src. there is a top-level scripts folder for those.
  • same for sample data
  • unclear to me why there is nfd under nfd_numpy

@Sam-Bouten
Copy link
Contributor Author

@Sam-Bouten Hey Sam, looks like some of the checks were not successful. Do you have a linter to tidy up the code?

The build linter takes issue with the spaces in formatted matrices.
Can I flag to ignore this lint for matrices specifically? If not, I will reformat.

        T = [[c2(γ),                c2(β),   1],
             [s2(γ),                s2(β),   0],
             [s(γ) * c(γ),   -s(β) * c(β),   0]]

@tomvanmele
Copy link
Collaborator

you can, as long as you keep it to a reasonable amount of exceptions...
in any case, the checks have to pass :)

@Sam-Bouten
Copy link
Contributor Author

  • example scripts should not be included in src. there is a top-level scripts folder for those.
  • same for sample data
  • unclear to me why there is nfd under nfd_numpy

Ok I'll flatten these nested folders. I was keeping nfd separate from fd,
but as I understand their scripts and data will live at the same level of the repo.

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