New D3 version

[pavgra]

• D3 was updated to Version 4.9.1
• Charts were moved into separate library, making it possible not to duplicate code in Atlas and Achilles

[chrisknoll]

Not sure I understand this: the jnj.chart is being updated in this local repo, but you're pointing the reference to jnj_chart to an external repo?

[pavgra]

As the charts functionality is required by both Atlas and Achilles, we moved it out into a separate module and therefore separate repo, and removed a local copy from Atlas'es repo (to make these projects reference the lib, not copy). In current state the library is just copy-pasted, which leads to unsync of code base in the projects using it, makes you to do double work when changes are required and is just a bad programming practice.

[chrisknoll]

Thanks @pavgra . The issue here then is where this common codebase lives. It shouldn't be removed from the repository under the OHDSI organization. If we are looking to create a shared repository of common code, you should speak with @pbr6cornell about setting up this shared repository, and then we can work together on moving the code into a form that is better suited to code reuse. But it's not appropriate to have the code moved from the OHDSI governance to some other organization without first asking the code owners first.

[t-abdul-basser]

Agreed. The library should be included locally (i.e. not externally) until we can move it to a OHDSI site. Since new functionality should be added to Atlas (Data Sources) not AchillesWeb which is in maintenance and not a focus of new features moving forward this should be fine.

[gklebanov]

@chrisknoll, @t-abdul-basser - agree with all your comments. This module was meant to stay in OHDSI repo. With all good intentions of creating a re-usable module, we should have done a better job in discussing everything in this group first - lesson learned.

[pavgra]

@chrisknoll, there was no idea to transfer this code outside. It was done in development purposes and as soon as we agree (and if we) on these improvements, we will transfer repo ownership to OHDSI, of course. However, I still defense the point that these changes are bringing valuable improvements into Atlas.

[t-abdul-basser]

These improvements are valuable. Thanks for contributing them to Atlas @pavgra :)

[pavgra]

@t-abdul-basser, @chrisknoll, so how can we solve the issue with module's repo? Will it be possible for someone, having permissions, to create an appropriate repo in OHDSI space? Then we can put module there and modify this Atlas'es pull-request

[fdefalco]

Absolutely, I can create a new repository for the shared library. What would we like to call it?
Definitely NOT jnj-charts. =)

[pbr6cornell]

this should do the trick: https://github.com/OHDSI/Visualizations

…

On Jul 2, 2017 2:28 AM, "Pavel Grafkin" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In js/main.js <#417 (comment)>: > @@ -103,7 +103,7 @@ requirejs.config({ "negative-controls": "components/negative-controls", "d3": "d3.min", "d3_tip": "d3.tip", - "jnj_chart": "jnj.chart", + "jnj_chart": "https://cdn.rawgit.com/odysseusinc/atlascharts/master/index", @t-abdul-basser <https://github.com/t-abdul-basser>, @chrisknoll <https://github.com/chrisknoll>, so how can we solve the issue with module's repo? Will it be possible for someone, having permissions, to create an appropriate repo in OHDSI space? Then we can put module there and modify this Atlas'es pull-request — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#417 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAsrGoRFpqgZM_0MPfaGOdkeEPal6XxKks5sJziUgaJpZM4OK8pf> .

[pavgra]

@pbr6cornell, I've created pull-request for Visualizations

[chrisknoll]

Hello, Everyone. Sorry for the late reply, I was out of touch for the past few days on vacation, and wasn't able to reply to this until now.

@pbr6cornell : I don't think we need a repo of visualizations, we need a repo to host a shared components that are shared across applications. jnj chart is one of the libraries, but the facet table component we have is also something that is shared between applications (I think we have seen it used in Achilles, atlas, Penelope and now in active surveillance). We could either have 1 repo of shared libraries with a set of different build scripts to create the individual libraries, or we could have 1 repo-per-library where each repo is dedicated to a patricular library (and has the benefit of a dedicated issue tracker for each library). Either way could work, but I don't think a library called 'visualization' to host the charting library is what we need in this case.

@pavgra: I appreciate your efforts on updating the jnj charting library to work with a newer D3 version, however there was another change that went unmentioned in your updates that I think I have a concern with: you made the individual plotting functions singleton-oriented instead of the way it was origionally designed 'instance oriented'. Ie:

var linePlot = new chart.line();
linePlot.render(...)

vs.

chart.line.render(...)

I don't want to go into the pluses and minuses here since I didn't think we would take this pull request as it was, but when we figure out the shared-component home, we can open up a thread about the changes there.

-Chris

[pavgra]

@chrisknoll, such approach to the charts code organization was applied due to their nature: they do not store or use any instance associated variables, so there is no need in instantiating a class every time you create a chart, charts' methods can be static because it is enough. Why to overcomplicate things until it's required?

[chrisknoll]

Hi, @pavgra.
I guess I'll go into the pluses and minuses here.... Firstly, I don't consider making the module return constructor functions 'overcomplicated'. It's a very natural pattern where the module returns the definitions of objects and the callers create instances, making the library work in a more object oriented manner.

It was also built in a forward-thinking manner: initially we didn't have a lot of parameters to pass into the charts for rendering, but as time went on, we identified more and more configuration parameters that we'd put into the function signature that made the library more cumbersome to use: axis parameters like ticks, formatting parameters, etc. I never got to updating the library to work more fluently but with constructor functions that return an instance, you can have the api work like this:

var linePlot = new chartLib.line();
linePlot.setAxis({...})
  .setFormat({...})
  .setTitle({..})
  .setXXX({..});

It can't operate like this with the change that you made.

Additionally, if we wanted to incorporate some kind of shared behavior between subclasses of charts (such as enabling pan-and-zoom functionality at a higher level) we'd be able to do this with implementing a base chart instance and have subclasses (line, boxplot, bar, etc) inherit the behavior.

Finally, I think you'd agree that most of the changes made in this PR would have been avoided if you stuck with the initial API design. We have other applications which consume this library (granted a copy of the library was made locally) but if we change the API in this way, then anywhere that wants to leverage the 'shared' codebase will have to go back and find all uses and make the same kind of changes you've made here. With due respect, I don't see that the change to a singleton as such an advantage that it's worth causing such a major change to the API.

[chrisknoll]

All-
I'm going to start a thread in the OHSDI forums to start the discussion about the details about how a shared component repository might work, and get some feedback about how this might work and what considerations are important to the interested developers. I'll post back here with a URL to the forum once it gets posted.

[chrisknoll]

I've made the post here:
http://forums.ohdsi.org/t/sharing-client-components-across-applications/3008

[fdefalco]

There are two things I'm reading in this thread. One is a discussion about the design approach of the library and the other is about how to organize a shared component.

With regards to the organization of shared components i replied to the forum thread stating my position (one repository per library).

I believe the upgrade of the d3 library is a valuable contribution to the charting library. I'm not opposed to the way the library is now structured in this PR and I have already seen it demonstrated in ATLAS as well as in the Arachne platform. I'm not opposed to the ongoing conversation about the preferred design approach, however, I would also like to ensure we're reviewing this PR with respect to functionality in addition to the design approach.

[chrisknoll]

I agree with @fdefalco that there's 2 concerns in this thread, and these should be addressed separately.

For this PR Re: adding d3: the api change to the library is not related to the version of the D3 library used. Changing the API signature of the charting module should not be part of a D3 upgrade. We use this charting API in different applications and if we want to just drop in the shared library, we should not change the API.

So, I can approve the D3 changes, but not the API changes. I recommend the changes to the charting API be reverted and the PR updated appropriately.

-Chris

[chrisknoll]

The new library defines a different datastructure than before: it used to take a series of objects with 3 properties:

• id
• label
• value

The new library expects a stucture with 2 fields in it:

• data : an array of values
• legend: an array of labels that correspond to the values

This makes code in Atlas fail (specifically: report-manager.js line 2735: mapConceptData() converts the data from the result into the structure the donut requires.

However, it appears that you did acoount for this in data-sources.js:

// result contains a set ob objects with id, value, label properties
			return {
				data: result.map(function(series) {
					return series.value;
				}),
				legend: result.map(function(series) {
					return series.label;
				})
			};

So it appears that the fix here is to add this map() call in the mapConceptData function in report-manager.js.

[chrisknoll]

I've merged this into a local branch for final inclusion into Atlas