JuliaPhysics / PeriodicTable.jl

Periodic Table for Julians! :fire:
Other
111 stars 26 forks source link

Add utility function to generate property list #40

Closed stefanbringuier closed 9 months ago

stefanbringuier commented 2 years ago

Problem

A frequent operation that is carried out using PeriodicTable.jl is to generate a list of element properties from Elements. This pull request adds a simple utility function to the src/PeriodicTable.jl module.

Solution

A new function getlist which returns a list when passed a field name variable (::Symbol or ::String) of Element which is in Elements.

The following are example method calls:

getlist(elements,:name)
...
getlist(elements[1:10],"symbol")
...
getlist(elements[[6,74]],:density)
...

Comments

The function/methods have been placed in PeriodicTable.jl module and exported from within that scope, but it may be more practical to put this in a submodule file Utilities.jl if more utility functions are added in the future.

carstenbauer commented 2 years ago

First of all, thanks for the PR!

However, I fail to see the actual advantage of having a dedicated getlist given that one can just use basic Julia features (broadcasting, getproperty / getfield) to achieve the same:

julia> getproperty.(elements, :name) == getlist(elements, :name)
true

julia> getproperty.(elements[[6,74]], :density) == getlist(elements[[6,74]],:density)
true

The only new "feature" here that I can see is support for Strings instead of Symbols which, IMHO, isn't worth it.

(One can't even really make the argument that accessing properties / fields of elements is bad style - because it could be an implementation detail that isn't part of the API - given that we explicitly document their existence and encourage their usage in the README)

But let's see what others think.

stefanbringuier commented 2 years ago

@carstenbauer thanks for the consideration. You're completely correct about this providing no true advantage, its merely for convenience and style. I was just looking to avoid broadcasting notation and add String support. Feel free to reject/close if no additional interest.

carstenbauer commented 9 months ago

I'll close this for now.