-
Notifications
You must be signed in to change notification settings - Fork 207
Doc Update for Opset Versioning Explanation #200
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
Conversation
vinitra-zz
commented
Dec 13, 2018
- added relevant links
- [to be edited/completed] explanation for opset versioning in ONNXMLTools conversion
- fixed various minor grammar and wording choices for readability
README.md
Outdated
|
|
||
| ## Acknowledgments | ||
| The package was developed by the following engineers and data scientists at Microsoft starting from winter 2017: Zeeshan Ahmed, Wei-Sheng Chin, Aidan Crook, Xavier Dupre, Costin Eseanu, Tom Finley, Lixin Gong, Scott Inglis, Pei Jiang, Ivan Matantsev, Prabhat Roy, M. Zeeshan Siddiqui, Shouheng Yi, Shauheen Zahirazami, Yiwen Zhu, Du Li, Xuan Li, Wenbing Li | ||
| This package was developed by the following engineers and data scientists at Microsoft starting from winter 2017: Zeeshan Ahmed, Wei-Sheng Chin, Aidan Crook, Xavier Dupre, Costin Eseanu, Tom Finley, Lixin Gong, Scott Inglis, Pei Jiang, Ivan Matantsev, Prabhat Roy, M. Zeeshan Siddiqui, Shouheng Yi, Shauheen Zahirazami, Yiwen Zhu, Du Li, Xuan Li, Wenbing Li, Vinitra Swamy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, we need remove the name list here. They are all the real names instead of the account ids.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed acknowledgements section with latest update. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wenbingl Why are these names removed? and what is wrong with real names? These are the people (except last 4) that initially developed this tool. We had listed their names here because they would never show up on github contributors since the repo was an internal VSTS repo when it was first developed, however, when it was open-sourced on Github for launch the VSTS history was wiped out. The folks that contributed to ONNXMLTools after the launch, their names will show up in github contributor list hence there is no need to list their names and expand this list.
The ethical thing to do would be to restore the original acknowledgment as below (which I can do):
The initial version of this package was developed by the following engineers and data scientists at Microsoft during winter 2017: Zeeshan Ahmed, Wei-Sheng Chin, Aidan Crook, Xavier Dupre, Costin Eseanu, Tom Finley, Lixin Gong, Scott Inglis, Pei Jiang, Ivan Matantsev, Prabhat Roy, M. Zeeshan Siddiqui, Shouheng Yi, Shauheen Zahirazami, Yiwen Zhu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting Microsoft and Real names together in a public place is a typical information leakage, which is major concern. So either we can keep "engineers and data scientists at Microsoft " or a real name list if you'd like to
'Ethical' is a strong word here since from the contribution list, the most changes is indicated as the original team member, nothing related to this PR owner or reviewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wenbingl Let me seek legal guidance and get back to you. Just want to do the ethical thing here .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A guidance will be better but I don't know if you can find one for this specific case. Or if you can tell the name list here actually are agreed with all members, you can also submit a PR to fix it.
|
ready to be merged? |
…f why target opset might be larger than the model's actual opset
Yes! @wenbingl, when you get a chance, please review my most recent explanatory/formatting changes. Sorry for the delay, and happy new year! |
wenbingl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the doc update.