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

Feature/upload #77

Merged
merged 6 commits into from
Nov 2, 2017
Merged

Feature/upload #77

merged 6 commits into from
Nov 2, 2017

Conversation

awilkey
Copy link
Contributor

@awilkey awilkey commented Oct 26, 2017

Add ability to upload local/remote files to the current view.

@awilkey awilkey requested a review from adf-ncgr October 26, 2017 20:47
@adf-ncgr
Copy link
Member

this is definitely not an absolute requirement, but would be kind of neat if not too hard to make work. I constructed a config file with no sources (and no initial maps in view, since there are no maps); the idea was that a person could just start with a blank app slate and just construct their
own "mashup" of data they know about (local or at some URL).
currently on legfed-dev you can access this as:
/cmap-js/?config=cmap.empty.json
this actually seemed to load without problems (I wasn't sure that it would), and I was able to
get to the menu for Add Data to Map but then when trying to actually import maps into
a new "mymaps" Target Map Set, I get an error (see below). Haven't looked further to see
if it is something easy to fix, but thought it's probably better for you to do so anyway...

VM313:1 Uncaught SyntaxError: Unexpected token u in JSON at position 0
at JSON.parse ()
at UploadDialog._onAddData (UploadDialog.js:37)
at HTMLButtonElement.onclick (UploadDialog.js:137)
at HTMLButtonElement.callback (mithril.js:915)

@adf-ncgr
Copy link
Member

test-add-data.2.txt
test-add-data.1.txt

here's something that is probably considered a bug in the current implementation. I tried adding a set of data to an existing map, and gave the features in the new dataset a different type to help distinguish the newly added from the existing features. However, the originally present features seem to then appear in the QTL tracks for both the original feature type and the new feature type, suggesting that they are somehow getting duplicated or maybe just getting multiple types associated. I know that description was probably clear as mud but try the simple test files attached here by first adding test-add-data.1.txt to a new target map set then adding the second file to that newly created target map set, then create QTL tracks for both SNP_1 and SNP_2 and see if you can reproduce what I'm trying to describe. then see if you can fix it, if you agree it is odd behavior.

@awilkey
Copy link
Contributor Author

awilkey commented Oct 31, 2017

The first point is due to current implementation assuming there is at least one map set to build off of, as it is infinitely easier to just clone and hijack an existing dataset than it is to build one from scratch, with a little work, it should be fixable.

The second, I can't replicate the behavior. Grabbed both sets of data and uploaded, which resulted in the following screencapture when adding QTL tracks:

screen shot 2017-10-31 at 11 05 48

edit: The tracks being QTL_1 pre-upload of QTL_2, QTL_2 and QTL_1 post upload from left to right.

@adf-ncgr
Copy link
Member

OK, trying to re-replicate I just realized that I got confused by the UI- I had earlier created a track that combined SNP_1 and SNP_2 features rather than creating separate tracks for them. sorry for the false alarm on that. Do you want to proceed with merge before addressing point 1? I can submit an enhancement request for that, it is certainly not high priority.

@awilkey
Copy link
Contributor Author

awilkey commented Oct 31, 2017

I'll look into the first note and see if I can get a fix up tomorrow, or merge and add an enhancement request, though probably the former.

@adf-ncgr
Copy link
Member

OK, sounds good.
Returning to something you had said in one of your emails about the initial implementation ('[items are] given a primary tag of "Uploaded"'); it seems like it is probably a good idea to have some way of tracking the origin of stuff that gets added into an existing target map set. using tags seems like a reasonable approach for now, maybe just adding something to the add Data UI to allow people to specify a tag value to be used in this case (so they can discriminate easily between various upload sources)? just a thought, if you have other ideas feel free to share...

@awilkey
Copy link
Contributor Author

awilkey commented Oct 31, 2017

An extra input box with tag-prefix would probably be easy enough to implement. Right now its just adding an "upload-" prefix to the value in feature_type_acc, which is the first spot looked for when making tags. When that title isn't there, like the the test files above, there is not tweak to the tag. (using a prefix instead of an outright replacement allows for uploaded data to be grabbed when making a qtl track of the same tag type alongside the regular data, as populating a qtl track is just based on regex-matching, without bothering to constrain a match at beginning-or-end... I consider this a feature, but it may be unwanted behavior.)

Copy link
Member

@adf-ncgr adf-ncgr left a comment

Choose a reason for hiding this comment

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

starting with empty config working well, thanks! A couple of little usability things for the Add Data feature:
-I accidentally added a file twice because the state of the Add Data menu isn't cleared after the last action; there might be some arguments for retaining last selection in case of error, and I can probably come up with use cases for adding the same data twice to different mapsets, but I think in terms of default behavior it's probably best not to retain the last input (except for maybe remembering the directory of the last file selection for when they go to choose a new file). let me know if you think otherwise (and this need not be a blocker)
-one thing that is a little confusing is when a remote URL is specified and the response has a little delay, the Add Maps appears as empty until the add map dialog is reopened after the request has been processed. Not sure this is worth bothering about, but I could imagine just making things block until the request has been successfully processed. Might also be good to assume that after requesting the Add Data they want to go directly to Add Maps, at least in the case where they've created a new target set. Not sure, maybe worth getting real user input on that.

anyway, looking good; feel free to merge unless you want to address any of the tweaks suggested first.

@awilkey
Copy link
Contributor Author

awilkey commented Nov 2, 2017

Added a quick tweak that should reset the state of the upload dialog to blank out when it is opened.
The second I am not sure of actual best behavior to do in this case, so I will leave it as is for the time being.

In an ideal future, it would probably be for the best to have an "advanced" toggleable div that allows users to specify configuration options for the new data and mark how they want the view set post upload.

@awilkey awilkey merged commit 26def77 into develop Nov 2, 2017
@awilkey awilkey deleted the feature/upload branch April 24, 2018 16:36
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