Skip to content

Conversation

@joewallwork
Copy link
Owner

This PR mainly exists to provide feedback,

Copy link
Owner Author

@joewallwork joewallwork left a comment

Choose a reason for hiding this comment

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

This looks great @acse-xt221, keep up the good work! I left some comments that will hopefully be useful.

@@ -0,0 +1,38 @@
from models.burgers_n2n import *
Copy link
Owner Author

Choose a reason for hiding this comment

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

If this file is identical to examples/burgers/config.py, perhaps you could avoid duplicating all these lines by just having the single line from examples.burgers.config import *?

@@ -0,0 +1,38 @@
from models.burgers_one2n import *
Copy link
Owner Author

Choose a reason for hiding this comment

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

Same again here.

@@ -0,0 +1,43 @@
from nn_adapt.layout import NetLayoutBase
Copy link
Owner Author

Choose a reason for hiding this comment

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

Similarly, you could plausibly import from examples/burgers/network.py, unless you plan to change the network architecture.

@@ -0,0 +1,43 @@
from nn_adapt.layout import NetLayoutBase
Copy link
Owner Author

Choose a reason for hiding this comment

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

And again.

MODEL = steady_turbine
NUM_TRAINING_CASES = 100
MODEL = burgers
NUM_TRAINING_CASES = 1
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is okay to start off with. However, further down the line I think it would be a good idea to use a larger number of cases.

Comment on lines 146 to 147
q_plus = solver_obj_plus.solution
# J = config.get_qoi(refined_mesh[-1])(q_plus[-1])
Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think you need these lines.

Comment on lines 200 to 204
V_plus = adj_sol_plus[step].function_space()
fwd_sol_plg = Function(V_plus)
tm.prolong(out["forward"][step], fwd_sol_plg)
adj_sol_plg = Function(V_plus)
tm.prolong(out["adjoint"][step], adj_sol_plg)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, this looks right.


# Evaluate errors
dwr += dwr_indicator(config, mesh, fwd_sol_plg, adj_error)
dwr_list.append(dwr_indicator(config, mesh, fwd_sol_plg, adj_error))
Copy link
Owner Author

Choose a reason for hiding this comment

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

Nice. So these can be used as the "target" data for each timestep.

return out if retall else out["dwr"]


def dwr_indicator(config, mesh, q, q_star):
Copy link
Owner Author

Choose a reason for hiding this comment

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

Again, if this is unmodified then you can just import it from solving.py, to avoid duplication.

self._solver.solve()


class Solver_one2n(nn_adapt.solving.Solver):
Copy link
Owner Author

Choose a reason for hiding this comment

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

If this is largely the same as the Solver class in examples.models.burgers then you could just define this to be a subclass of it. That way, you only need to write out the methods that are different and don't need to repeat the others.

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