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

Related Jobs in Dataset and Model view #767

Merged
merged 1 commit into from May 23, 2016
Merged

Related Jobs in Dataset and Model view #767

merged 1 commit into from May 23, 2016

Conversation

TimZaman
Copy link
Contributor

See #667 , revised version of #668.
See below for screenshots. Very straightforward.

Ready for review @lukeyeager (#667 (comment))

Dataset View:

image

Model View:

image

elif isinstance(job, generic.GenericDatasetJob):
return generic.views.show(job)
return generic.views.show(job, related_jobs=related_jobs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to add this parameter to the show() function in digits/dataset/generic/views.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that's because the file digits/dataset/generic/views.py is brand new. Didn't catch that, well spotted.

@TimZaman
Copy link
Contributor Author

TimZaman commented May 23, 2016

Excellent points, I reduced down the complexity to just one function. I hope this feature will increase the ease for work for us Digitizers.
Looks good to me.
Ready for re-review, @gheinrich .

@gheinrich
Copy link
Contributor

gheinrich commented May 23, 2016

... Digitizers.

Wow. Nice! 🤘

related_jobs = []

if isinstance(job, ModelJob):
datajob = job.dataset
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the updates! If you don't mind a couple more comments... there you can do:

related_jobs.append(datajob)

so you can take lines 233-235 out.

@TimZaman
Copy link
Contributor Author

Keep going like this and we will run out of lines of code. Squashed yet again plz review @gheinrich .

<div class="panel-group">
<!-- Related model jobs -->
{% for r_job in related_jobs %}
{% if prev_job_model != r_job.job_type() %}
Copy link
Contributor

Choose a reason for hiding this comment

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

it might have been more explicit to call this prev_job_type but I don't think it's a big deal

@gheinrich
Copy link
Contributor

Great PR, very useful! I tested it (also on some "generic" data jobs) and it works well. Thanks!

@TimZaman
Copy link
Contributor Author

Perfect, thanks for the review.

@TimZaman
Copy link
Contributor Author

Okay, updated the line. Want me to squash?

@lukeyeager
Copy link
Member

Okay, updated the line. Want me to squash?

Yep, looks good to me!

Reduced complexity of implementation

Newline fixes

Removed a print

Fixes

Fixed link for future resilience

Typo
@TimZaman
Copy link
Contributor Author

Squashed.
Maybe for another time, and depending on how you guys like this in practice (we use this a lot); it might even be better to put in a little @jmancewicz -style jquery table.

@lukeyeager lukeyeager merged commit 9b69c01 into NVIDIA:master May 23, 2016
@lukeyeager
Copy link
Member

It's kinda weird that the current model shows up below as a "related" job, no? I can't see why I would ever click on that.

SlipknotTN pushed a commit to cynnyx/DIGITS that referenced this pull request Mar 30, 2017
Related Jobs in Dataset and Model view
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants