Light refactor to improve development #305
Conversation
JustinBakerMath
added
documentation
| self.input_args | ||
| == "inv_node_feat, equiv_node_feat, edge_index, edge_attr" | ||
| ) | ||
| assert self.conv_args == "inv_node_feat, edge_index, edge_attr" |
There was a problem hiding this comment.
Why do some stacks has this assert and others not?
There was a problem hiding this comment.
We can add them everywhere or remove them all. I just added this for security.
There was a problem hiding this comment.
I am partial to removing them all since they are set just before, but could be convinced either way.
|
I've cloned and working on revising the incorporating the MACEStack. It looks like I have commit ability to your branch here. Do you have a preference for me PR'ing to your branch or directly committing is fine? |
I am ok with whatever your preference is. It makes sense to add it here. |
|
Thanks for yalls patience, I need to polish up MACE in other ways but it's best to leave that to do separate PR. Assuming MACE passes the checks, I believe it's integrated into the format here. |
|
|
||
| ### There is a readout before the first convolution layer ### | ||
| outputs = [] | ||
| ### MACE has a readout block before convolutions ### | ||
| output = self.multihead_decoders[0]( |
There was a problem hiding this comment.
Drawing your attention to this part. I moved it back since it is actually an output that is used for prediction, but let me know if there was a specific reason you moved this to embedding that still holds.
There was a problem hiding this comment.
This makes sense to me.
|
Is there anything left to adjust before tagging Max? |
|
Hi Rylie,
Please go ahead.
…On Fri, Nov 1, 2024 at 6:03 PM RylieWeaver ***@***.***> wrote:
@JustinBakerMath <https://github.com/JustinBakerMath> @ArCho48
<https://github.com/ArCho48>
Is there anything left to adjust before tagging Max?
—
Reply to this email directly, view it on GitHub
<#305 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMSKZ5O7TNXH6LVYF4G6RLTZ6P3DZAVCNFSM6AAAAABQ6WZH2GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJSGY2TAMZRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
* light refactor to improve development * removed printing * testing my commit ability * reformat MACE * remove test change * fix error relating to readout block dimension input --------- Co-authored-by: Justin <[email protected]> Co-authored-by: Rylie Weaver <[email protected]>
* light refactor to improve development * removed printing * testing my commit ability * reformat MACE * remove test change * fix error relating to readout block dimension input --------- Co-authored-by: Justin <[email protected]> Co-authored-by: Rylie Weaver <[email protected]>
* light refactor to improve development * removed printing * testing my commit ability * reformat MACE * remove test change * fix error relating to readout block dimension input --------- Co-authored-by: Justin <[email protected]> Co-authored-by: Rylie Weaver <[email protected]>
PR aimed at improving signatures and variable names to enhance model development.
Base.__init__(input_args, conv_args, ...)requires arguments to be specified in the input to the base class. This enables better wrapper development by instantiatingself.input_args, self.conv_argsamong all classes inheriting fromBase. 1edge_attradding to the arguments ifuse_edge_attris true 2._conv_argshas been changed to_embeddingto handle embedding of equivariant information. This is inline with the recent changes inPAINNand fits closely to the actual operations of this function. 3x, postoinv_node_feat, equiv_node_feat. The original architectures treatedxas invariant information andposas equivariant information. To improve the transparency of these variable names they have been adjusted. 4Basee.g.PAINNandPNAEqnow share the same forward call asBase. 5