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

Nodal to Harmonic Basis #62

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

Conversation

AdelekeBankole
Copy link
Collaborator

No description provided.

@jeremylt
Copy link
Owner

Something looks strange here - you have a bunch of old commits that got squashed out of main

@AdelekeBankole
Copy link
Collaborator Author

@jedbrown, pls what can I try to clean this up. This keeps giving me copied commits, which I don't want

@AdelekeBankole
Copy link
Collaborator Author

Yeah @jeremylt. I need to clean this up. I have been trying to avoid this and it happened again.

@jedbrown
Copy link
Collaborator

Do you know how to solve this problem or think you can figure it out? I see lots of conflicts here. What changes are intentional? Are they contained in a few commits you can cherry-pick or retain in a git rebase -i?

@AdelekeBankole
Copy link
Collaborator Author

Only the last three commits are intentional

@jedbrown jedbrown force-pushed the ade/nodal-to-harmonic-basis branch from 112fe06 to bc0075a Compare October 21, 2022 03:40
@jedbrown
Copy link
Collaborator

I reset your branch, with just the three commits cleanly applied on top of main.

Comment on lines +87 to +96
plot(θ / π, eigenvalues ./ π, linewidth = 3) # Dispersion
plot!(
identity,
xlabel = "θ/π",
ylabel = "Eigenvalues",
label = "exact",
legend = :none,
color = :black,
title = "P=$P, collocate=$collocate, τ=$τ",
)
Copy link
Owner

Choose a reason for hiding this comment

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

Manual plotting shouldn't be part of these examples - only the Jupyter examples

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I am aware of that, it is only for tentative experimental purposes, it will be eventually removed.

@@ -92,6 +92,7 @@ mutable struct Operator
multiplicity::AbstractArray{Float64}
rowmodemap::AbstractArray{Float64,2}
columnmodemap::AbstractArray{Float64,2}
qtbtd::AbstractArray{Float64,2}
Copy link
Owner

Choose a reason for hiding this comment

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

Can we pick a less cryptic name? Is this for half assembly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ja genau, qtbtd seems too cryptic

@jeremylt
Copy link
Owner

Note - CI was changed on the main branch, so this branch will need to be rebased in order to be mergable

@AdelekeBankole
Copy link
Collaborator Author

Ok, thanks for the head up.

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