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

input_master.py refactor proposal using GMTK interface #144

Closed

Conversation

aparnagopalakrishnan7
Copy link

Updated NameCollection, Mean, MC, MX, DPMF, Covar types. In progress: DenseCPT, VE, RealMat

@EricR86
Copy link
Member

EricR86 commented Sep 1, 2020

Could you first the linting/build errors reported by the github checks (found by PyFlakes and running ./test_all.sh in the test directory) and then notify me and I'll re-review?

Copy link
Member

@EricR86 EricR86 left a comment

Choose a reason for hiding this comment

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

Let me know when you've made these changes. Thanks!

"""
returns: iterable of strs containing GMTK parameter objects starting
with names
Return list of head track names.
Copy link
Member

Choose a reason for hiding this comment

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

Returns the first track name in each track group

type_name = "DT"
copy_attrs = ParamSpec.copy_attrs + ["seg_countdowns_initial",
"supervision_type"]
def generate_gmtk_obj_names(self, obj, track_names):
Copy link
Member

Choose a reason for hiding this comment

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

generate_gmtk_object_names

type_name = "DT"
copy_attrs = ParamSpec.copy_attrs + ["seg_countdowns_initial",
"supervision_type"]
def generate_gmtk_obj_names(self, obj, track_names):
Copy link
Member

Choose a reason for hiding this comment

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

Why is track_names used here? There's self.track_names and even self.track_groups? Is this called multiple times across track groups?

MX: "mx"
MC: "mc_diag", "mc_gamma", "mc_missing", "gammascale"
DPMF: "dpmf"
:param obj: str: type of gmtk object for which names must be generated
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of obj it should be renamed to gmtk_object_type

"""
Generate GMTK object names for the types:
NameCollection: "col"
entries in NameCollection: "mx_name"
Copy link
Member

Choose a reason for hiding this comment

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

It's not actually "mx_name" though right? can you describe this better somehow?

Choose a reason for hiding this comment

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

This has been renamed to "collection_entries" now.

segway/input_master.py Show resolved Hide resolved
dpmf_names *= multiple # replicate dpmf names for iteration

# create MX objects
for i in range(len(names)):
Copy link
Member

Choose a reason for hiding this comment

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

use zip(names, dpmf_names, mc_names) to iterate over instead

dpmf_values = str(round(1.0 / self.num_mix_components,
ROUND_NDIGITS))
# create dpmf objects
for i in range(len(names)):
Copy link
Member

Choose a reason for hiding this comment

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

zip(names, values) and for all other instances you can find as well

@@ -348,6 +401,10 @@ def calc_prob_transition(self, length):
def make_dense_cpt_segCountDown_seg_segTransition(self): # noqa
# first values are the ones where segCountDown = 0 therefore
# the transitions to segTransition = 2 occur early on

# see Segway paper
Copy link
Member

Choose a reason for hiding this comment

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

put the citation in comments (in brackets perhaps)

segway/input_master.py Show resolved Hide resolved
"covar", "collection_names", "collection_entries", "dpmf", "gammascale",
"gammashape", "tied_covar"]

if not gmtk_object_type in allowed_types:
Copy link
Member

Choose a reason for hiding this comment

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

allowed_types is not necessary here. You can check if the key exists in GMTK_OBJECT_NAME_GENERATORS

segway/input_master.py Show resolved Hide resolved
head_track_names = self.get_head_track_names()
names = []
for name in head_track_names:
names.append("collection_seg_{}".format(name))
Copy link
Member

Choose a reason for hiding this comment

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

For all the GMTK object name format strings, you should define them as constants above. e.g. NAME_COLLECTION_FORMAT_STRING = "collection_seg_{}"

segway/input_master.py Show resolved Hide resolved
@EricR86
Copy link
Member

EricR86 commented Mar 3, 2023

This work is continued in PR #163

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.

2 participants