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

Update Frames.jl #533

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update Frames.jl #533

wants to merge 1 commit into from

Conversation

tkrisnguyen
Copy link
Contributor

Propose a solution to issue #532

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.13%. Comparing base (7e0323b) to head (4c65d72).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
+ Coverage   85.11%   85.13%   +0.02%     
==========================================
  Files          45       45              
  Lines        1935     1938       +3     
==========================================
+ Hits         1647     1650       +3     
  Misses        288      288              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mvanzulli mvanzulli left a comment

Choose a reason for hiding this comment

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

Makes sense, maybe we can avoid using unicode for subindex as it ads an extra overhead with no much value. So can we just rename to use n1 and n2

function Frame(n₁::N1, n₂::N2, g::G, mass_matrix::MassMatrix = Consistent,
label::Label = NO_LABEL) where {N1 <: AbstractNode, N2 <: AbstractNode, G <: AbstractCrossSection}
temp = promote(n₁.x,n₂.x)
n1r = Node(temp[1],n₁.dofs)
Copy link
Member

Choose a reason for hiding this comment

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

Note that we will always allocate a new node, so maybe we can so it only when the numeric types missmatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a new node is allocated as proposed, does the performance reduce? I don't know how to check wether the numeric types are mismatch. Please help me.

Copy link
Member

Choose a reason for hiding this comment

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

Sure the cleanest way is to extract the types form Node object as it is parametric in the coords type:

julia> n2
• Node at [2, 3, 4] and dofs {}

julia> typeof(n2)
Node{3, Int64}

So we can use:

function Frame(n1::N1, n2::N2, g::G, mass_matrix::MassMatrix = Consistent,
        label::Label = NO_LABEL) where {dim, T1, T2, N1 <: AbstractNode{dim, T1},
        N2 <: AbstractNode{dim, T2}, G <: AbstractCrossSection}
    if T1 == T2
        Frame(SVector(n1, n2), g, mass_matrix, label)
    else
        temp = promote(n1.x, n2.x)
        n1r = Node(first(temp), n1.dofs)
        n2r = Node(last(temp), n2.dofs)
        Frame(SVector(n1r, n2r), g, mass_matrix, label)
    end
end

Next time it would be easier for me if you create this branch from your local desktop git https://github.com/apps/desktop if not I have to copy and paste your chages to reproduce what you did. I'm happy to help if u found any trouble with this.

function Frame(n₁::N, n₂::N, g::G, mass_matrix::MassMatrix = Consistent,
label::Label = NO_LABEL) where {N <: AbstractNode, G <: AbstractCrossSection}
Frame(SVector(n₁, n₂), g, mass_matrix, label)
function Frame(n₁::N1, n₂::N2, g::G, mass_matrix::MassMatrix = Consistent,
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Can you add a test in frame.jl with this case just creating single element ?

@mvanzulli
Copy link
Member

@tkrisnguyen are you planning to address this issue ? I'm happy to do a sync if that helps, if not I can try to merge it.

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.

2 participants