PeriodicTable.jl icon indicating copy to clipboard operation
PeriodicTable.jl copied to clipboard

Add utility function to generate property list

Open stefanbringuier opened this issue 2 years ago • 2 comments

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.

stefanbringuier avatar Nov 02 '21 01:11 stefanbringuier

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.

carstenbauer avatar Nov 03 '21 07:11 carstenbauer

@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.

stefanbringuier avatar Nov 03 '21 16:11 stefanbringuier

I'll close this for now.

carstenbauer avatar Jan 05 '24 09:01 carstenbauer