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

Added ArgMax operator #209

Merged

Conversation

Swopper050
Copy link
Collaborator

@Swopper050 Swopper050 commented Oct 13, 2024

Closes #14
Closes #210

@Swopper050 Swopper050 self-assigned this Oct 13, 2024
@Swopper050
Copy link
Collaborator Author

@wipsel What should we do about the string constant lint error?

The attributes 'axis' and 'keepdims' occur in (at least) 3 operators, causing the linter to say it should be a constant. However, it does not really feel nice to group some of the attributes together and some not (like select_last_index in this operator). Do you have an idea how we could do this?

Makefile Outdated Show resolved Hide resolved
@wipsel
Copy link
Collaborator

wipsel commented Oct 15, 2024

@wipsel What should we do about the string constant lint error?

The attributes 'axis' and 'keepdims' occur in (at least) 3 operators, causing the linter to say it should be a constant. However, it does not really feel nice to group some of the attributes together and some not (like select_last_index in this operator). Do you have an idea how we could do this?

I see two options. Make all attribute names a constant in their operator. That would make the linter happy and i don't really see a problem with it. It could be a nice way to add documentation about the attribute if need be.

Another option would be to create an attrs or attrname package defining the attribute names of all operators "globally"
The usage would than look something like this:

// in attrs/const.go
const (
    KeepDims = "keep_dims"
    Axis = "axis"
)

// in some operator 
import attrs


// Init initializes the argmax operator.
func (a *ArgMax) Init(n *onnx.NodeProto) error {
	attributes := n.GetAttribute()
	for _, attr := range attributes {
		switch attr.GetName() {
		case attrs.Axis:
                     // do  attr stuff
               }
       } 
}

I don't know how much of the attr names are shared and if it justifies a seperate package. Maybe you have a better view on this. If not you could define them under their own name under their own operator. I would however make it consistent with all the attr names for the same operator. So if you change them to a const for this i would include the select_last_index as wel. Don't know if it all makes sense but hope it helps

@Swopper050
Copy link
Collaborator Author

I see two options. Make all attribute names a constant in their operator. That would make the linter happy and i don't really see a problem with it. It could be a nice way to add documentation about the attribute if need be.

Let's do this. I created an issue #211 for it and for this MR only fixed it in ArgMax.

@Swopper050 Swopper050 merged commit 7ed9d6f into AdvancedClimateSystems:develop Oct 21, 2024
4 checks passed
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.

Update golangci-lint version Implement Argmax operator
2 participants