Skip to content
This repository was archived by the owner on Nov 17, 2023. It is now read-only.

Add TypeFlag=>string macro #16881

Merged
merged 6 commits into from
Jan 12, 2020
Merged

Conversation

Kh4L
Copy link
Contributor

@Kh4L Kh4L commented Nov 21, 2019

Description

Add a macro mapping mshadow type_flag to strings, to improve debuggability.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • Changes are complete (i.e. I finished coding on this PR)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Signed-off-by: Serge Panev <[email protected]>
@wkcn
Copy link
Member

wkcn commented Nov 22, 2019

I prefer to add the type name in DataType class, and get the type name from ‘mshadow::DataType<DType>::kName’.
https://github.com/apache/incubator-mxnet/blob/master/3rdparty/mshadow/mshadow/base.h#L321

@Kh4L
Copy link
Contributor Author

Kh4L commented Nov 22, 2019

I prefer to add the type name in DataType class, and get the type name from ‘mshadow::DataType::kName’.
https://github.com/apache/incubator-mxnet/blob/master/3rdparty/mshadow/mshadow/base.h#L321

AFAIK, we can't change /mshadow/base.h, as it is part of mshadow, that is archived.

@Kh4L Kh4L force-pushed the better_mshadow_type_print branch 2 times, most recently from 8e7d963 to 3acd71e Compare November 22, 2019 10:48
Signed-off-by: Serge Panev <[email protected]>
@Kh4L Kh4L force-pushed the better_mshadow_type_print branch from 3acd71e to a31b222 Compare November 22, 2019 11:30
@wkcn
Copy link
Member

wkcn commented Nov 23, 2019

Hi @Kh4L,
mshadow is not archived. It has been a part of MXNet, and we can modify it in MXNet repository.
#15600

Copy link
Contributor

@haojin2 haojin2 left a comment

Choose a reason for hiding this comment

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

@Kh4L
Copy link
Contributor Author

Kh4L commented Nov 27, 2019

@wkcn @haojin2 understood, I will change it to use this

Copy link
Member

@wkcn wkcn left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@Kh4L Kh4L requested a review from haojin2 November 27, 2019 23:59
@wkcn
Copy link
Member

wkcn commented Dec 5, 2019

Hi @haojin2 , the request has been addressed. Could you please help take a review? Thank you!

@wkcn
Copy link
Member

wkcn commented Jan 6, 2020

Hi @haojin2

@haojin2
Copy link
Contributor

haojin2 commented Jan 6, 2020

@Kh4L @wkcn What is the reason for moving the exisiting function to mshadow?

@wkcn
Copy link
Member

wkcn commented Jan 6, 2020

@haojin2 The reason is that the dtypes belong to mshadow, and it will be more convenient to get the name of mshadow::dtype.

@Kh4L
Copy link
Contributor Author

Kh4L commented Jan 6, 2020

@haojin2 that's right, as @wkcn , I moved it to mshadow for consistecy, where most of dtype routines are defined.

@haojin2 haojin2 merged commit 8accaca into apache:master Jan 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants