Skip to content

Conversation

ka-bear
Copy link

@ka-bear ka-bear commented Jul 10, 2024

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Added PIPNs to NeuralPDE
Fixes: #495

@sathvikbhagavan sathvikbhagavan changed the title Issue #495 Physics Informed Point Net Jul 16, 2024
end
end

if GROUP == "All" || GROUP == "PIPN"
Copy link
Member

Choose a reason for hiding this comment

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

The .github/workflows/ci.yml needs to add a PIPN group as well for this to run.

@ChrisRackauckas
Copy link
Member

Looks almost good to go! Left a few comments.

@ChrisRackauckas
Copy link
Member

Rebase

@sathvikbhagavan
Copy link
Member

Thanks! I will take a look as well in a couple of days.

$(DocStringExtensions.README)
"""
module NeuralPDE
module NeuralPDE
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
module NeuralPDE
module NeuralPDE

include("dgm.jl")

export NNODE, NNDAE,
export NNODE, PIPN, init_pipn_params, NNDAE,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export NNODE, PIPN, init_pipn_params, NNDAE,
export NNODE, PIPN, init_pipn_params, NNDAE,

???
"""
struct LogOptions
struct LogOptions
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
struct LogOptions
struct LogOptions

Comment on lines +560 to +561


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

## Positional Arguments
* `chain`: a Lux chain specifying the overall network architecture. The input and output dimensions
Copy link
Member

Choose a reason for hiding this comment

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

Is this even needed as shared mlps are constructed inside?

global_feature_repeated = repeat(global_feature, 1, size(point_features, 2))
combined_features = vcat(point_features, global_feature_repeated)
combined_features, st3 = Lux.apply(model.after_pool_mlp, combined_features, ps.after_pool_mlp, st.after_pool_mlp)
output, st4 = Lux.apply(model.final_layer, combined_features, ps.final_layer, st.final_layer)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't the original architecture also have another block of shared mlps?

flat_ps = ComponentArray(ps)
new_flat_ps = typeof(flat_ps)(θ)
return ComponentArray(new_flat_ps)
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
end
end

@@ -1,4 +1,4 @@
using Pkg
using Pkg
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using Pkg
using Pkg


@test prob isa OptimizationProblem
end
end No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
end
end

strategy = GridTraining(0.1)
discretization = PhysicsInformedNN(chain, strategy)

prob = discretize(pde_system, discretization)
Copy link
Member

Choose a reason for hiding this comment

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

Is it working with solve?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it should work

Copy link
Member

Choose a reason for hiding this comment

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

Ok, cool. Can you add it to the test and also address other comments?

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.

Physics-informed pointnet
3 participants