Mode enum #407

Jingyi99 · 2019-05-22T18:06:04Z

Issue: #400
I was not entirely sure where to put the mode enum class so I put it in hammer_tool.py. I can move it if there is a better place.
I will modify the string comparisons and open another PR if this enum class looks good.

edwardcwang

Thanks for your contribution! I left a comment below about this enum.

edwardcwang · 2019-05-22T18:14:49Z

src/hammer-vlsi/hammer_vlsi/hammer_tool.py

+    Empty = 2
+    Manual = 3
+    Generate = 4
+    Generated = 5


As far as I'm aware, the modes we use are:

Auto = 1 Empty = 2 Manual = 3 Generated = 4

append, prepend are part of another part of Hammer and not part of this mode enum.

Also, if we are going to create this enum, I think it may be good to also use them in some of the settings.

We should change power straps to generated from generate if we're going to use generate. I personally like generate better, but generated does keep all of the modes as adjectives, which also makes sense.

Yeah, maybe we can do that as a separate issue then?

We can just tack it on to #397 .

Hi Edward, thanks for your comment! I checked defaults.yml and it seems that pin place mode uses generated, and power straps mode uses generate. Just to make sure, they are the same and we'll stick with generate, right?
Also, I'm not sure what it means to use the enum in the settings, could you explain a bit more?

Hi Jingyi- let's use generated instead of generate. We'll change the power straps mode to generate.

Yeah, so John's comment above is alluding to that problem. I think the desired action is to use generated and just ignore the power straps mode for now.

As for use, if you look in the code, right now it uses strings - the intention is to use this enum instead

hammer/src/hammer-vlsi/hammer_vlsi/hammer_tool.py

Line 896 in 8bcbc40

if bumps_mode == "empty":

Jingyi99 · 2019-05-22T20:11:02Z

It seems like I broke something...

edwardcwang

For type hinting comments, let's keep them proper

edwardcwang · 2019-05-22T20:10:15Z

src/hammer-vlsi/hammer_vlsi/hammer_tool.py

@@ -970,7 +973,7 @@ def get_gds_map_file(self) -> Optional[str]:
        :return: Fully-resolved path to GDS map file or None.
        """
        # Mode can be auto, empty, or manual
-        gds_map_mode = str(self.get_setting("par.inputs.gds_map_mode"))  # type: str
+        gds_map_mode = ModeType.from_str(str(self.get_setting("par.inputs.gds_map_mode")))  # type: ModeType Enum


src/hammer-vlsi/hammer_vlsi/hammer_tool.py

edwardcwang · 2019-05-22T20:10:32Z

src/hammer-vlsi/hammer_vlsi/hammer_vlsi_impl.py

@@ -1323,15 +1323,15 @@ def generate_power_spec_commands(self) -> List[str]:
            return []

        power_spec_contents = ""  # type: str
-        power_spec_mode = str(self.get_setting("vlsi.inputs.power_spec_mode"))  # type: str
-        if power_spec_mode == "empty":
+        power_spec_mode = ModeType.from_str(str(self.get_setting("vlsi.inputs.power_spec_mode")))  # type: ModeType Enum


Co-Authored-By: edwardcwang <[email protected]>

jwright6323 · 2019-05-22T20:25:04Z

src/hammer-vlsi/hammer_vlsi/hammer_tool.py

@@ -970,7 +973,7 @@ def get_gds_map_file(self) -> Optional[str]:
        :return: Fully-resolved path to GDS map file or None.
        """
        # Mode can be auto, empty, or manual
-        gds_map_mode = str(self.get_setting("par.inputs.gds_map_mode"))  # type: str
+        gds_map_mode = ModeType.from_str(str(self.get_setting("par.inputs.gds_map_mode")))  # type: ModeType


We may want to create a static helper method called ModeType.from_setting that takes the HammerTool object and the setting string as args and returns a ModeType. e.g.

class ModeType: @staticmethod def from_setting(tool: HammerTool, setting: str) -> ModeType: return ModeType.from_str(str(tool.get_setting(setting)))

which would then allow the following change (no need for type hint, either):

Suggested change

gds_map_mode = ModeType.from_str(str(self.get_setting("par.inputs.gds_map_mode"))) # type: ModeType

gds_map_mode = ModeType.from_setting(self, "par.inputs.gds_map_mode")

Not to nitpick too much - at that point I would prefer almost gds_map_mode = self.get_mode_setting("par.inputs.gds_map_mode") to avoid a dependency from ModeType -> HammerTool

I also like that, perhaps more.

Jingyi99 · 2019-05-24T20:02:35Z

Sorry for introducing bugs. The errors in the unit tests are

File "/buildbot/run_unittests/build/src/hammer-vlsi/hammer_vlsi/hammer_tool.py", line 1135, in HammerTool
  def get_mode_setting(self, setting: str) -> ModeType:
NameError: name 'ModeType' is not defined

Is it potentially because I was not importing properly?
Also, are there any units tests that I can run locally?
Thanks!

jwright6323 · 2019-05-24T21:12:59Z

Hi Jingyi- you can run the unittests in src/test/unittests.sh: https://github.com/ucb-bar/hammer/blob/master/src/test/unittests.sh
You cal also run the typechecker by running the mypy_with_exclusions.sh script

Jingyi99 added 3 commits May 21, 2019 12:12

Replace blank power straps mode with empty

0fbcfa5

Merge branch 'master' of https://github.com/ucb-bar/hammer

cdad2aa

add Mode Enum class #400

46b4868

Jingyi99 requested a review from edwardcwang May 22, 2019 18:06

Jingyi99 added very low priority No workaround needed, not urgent, no timeline needed enhancement labels May 22, 2019

edwardcwang requested changes May 22, 2019

View reviewed changes

Jingyi99 added 2 commits May 22, 2019 13:08

include 4 mode types, change string comparisons to Enum comparisons

bbf024e

fixed typo

10d94d0

edwardcwang requested changes May 22, 2019

View reviewed changes

Jingyi99 and others added 3 commits May 22, 2019 13:13

Update src/hammer-vlsi/hammer_vlsi/hammer_tool.py

ba8f207

Co-Authored-By: edwardcwang <[email protected]>

Update src/hammer-vlsi/hammer_vlsi/hammer_tool.py

c2d56e5

Co-Authored-By: edwardcwang <[email protected]>

fix type hinting comments

a82a9b6

jwright6323 requested changes May 22, 2019

View reviewed changes

add hammer tool class method get_mode_setting

397deb7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mode enum #407

Mode enum #407

Jingyi99 commented May 22, 2019 •

edited

Loading

edwardcwang left a comment

edwardcwang May 22, 2019

jwright6323 May 22, 2019

edwardcwang May 22, 2019

jwright6323 May 22, 2019

Jingyi99 May 22, 2019

jwright6323 May 22, 2019

edwardcwang May 22, 2019

Jingyi99 commented May 22, 2019

edwardcwang left a comment

edwardcwang May 22, 2019

edwardcwang May 22, 2019

jwright6323 May 22, 2019

edwardcwang May 22, 2019

jwright6323 May 22, 2019

Jingyi99 commented May 24, 2019

jwright6323 commented May 24, 2019

	gds_map_mode = ModeType.from_str(str(self.get_setting("par.inputs.gds_map_mode"))) # type: ModeType
	gds_map_mode = ModeType.from_setting(self, "par.inputs.gds_map_mode")

Mode enum #407

Are you sure you want to change the base?

Mode enum #407

Conversation

Jingyi99 commented May 22, 2019 • edited Loading

edwardcwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jingyi99 commented May 22, 2019

edwardcwang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jingyi99 commented May 24, 2019

jwright6323 commented May 24, 2019

Jingyi99 commented May 22, 2019 •

edited

Loading