-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: main
Are you sure you want to change the base?
nfd_solver #2
Conversation
@Sam-Bouten Hey Sam, looks like some of the checks were not successful. Do you have a linter to tidy up the code? |
There was a problem hiding this 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 asnumpy
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
Some thoughts for discussion:
Currently there are global python for-loops in the algorithm: Another way would be to have the NaturalFace and NaturalEdge as flyweights; changing their instance methods
Currently the mesh vertex attribute 'is_anchor' is hardcoded as a flag to detect fixed vertices.
Should these functions reside in a more general location? |
There was a problem hiding this 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-levelscripts
folder for those. - same for sample data
- unclear to me why there is
nfd
undernfd_numpy
The build linter takes issue with the spaces in formatted matrices. T = [[c2(γ), c2(β), 1],
[s2(γ), s2(β), 0],
[s(γ) * c(γ), -s(β) * c(β), 0]] |
you can, as long as you keep it to a reasonable amount of exceptions... |
Ok I'll flatten these nested folders. I was keeping nfd separate from fd, |
1f20dfc
to
3375864
Compare
verbose docs
9072351
to
4364378
Compare
include natural force density solver module, and example scripts.