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

#28 edit chart menu #315

Open
wants to merge 152 commits into
base: v2.1
Choose a base branch
from
Open

#28 edit chart menu #315

wants to merge 152 commits into from

Conversation

cameronjtoy
Copy link
Collaborator

Changes:
UI is reflected in the new v2.0 Figma UI, which can change Chart Customization or Line Customization.
UI includes Category in frontend
Filter Parameter now includes a dropdown for numerical values to be specific with what is being filtered, not just the x-axis.
All types of chart types are handled in the ChartEditor class
The number of rows for Line Customization is determined by (dataset * ys * categories)
New Color Parameter to select color for lines/points/bars

JasonMai11 and others added 30 commits October 17, 2023 21:10
Changes:
- Profiler is now moved to Artifact.scala.
- Added info.vizierdb.profiler package.
- Artifact.updateDatasetProperty now includes "is_profiled" as a property.
Debrief:

In Artifact.scala, a property was added to the artifact using updateDatasetProperty.

- if "is_profiled" is not a property then it will be attached to the artifact.
- if "is_profiled" is a property then the profiler will run ONLY IF it is set to the boolean FALSE.

Once the profiler runs then the profiled information is then set to the value of the property "is_profiled". This will prevent the profiler from running multiple times on a single dataset.

*TEST CASES*:
Test cases are held in PublishArtifactSpec.scala. You must run the whole PublishArtifactSpec.scala otherwise the second profiler test case will fail. It will fail since the profiler has not been set as a property (which is done in the first profiler test case).
Changed datasetProperty so "Is_profiled" would be connected to the jsonObject. No longer determined by boolean. (**Only Temporary, will get changed so "is_profiled" is back to boolean**)
forceProfile parameter in describe and in datasetData is now set to True
"is_profiled" and profiler properties are now added to Artifact. Tests are now more accurate and test for correct information.
…ted ListObjects, Made some adjustments towards Backend towards fixing multiple Y-axis implementation
yIndex from Int -> Seq[Int]
VegaChart Change: added Offset between bars.

PlotUtils: Handles a Sequence of Y's correctly

BarChart: Adjusted to handle multiple Y's
@cameronjtoy cameronjtoy self-assigned this Apr 26, 2024
@okennedy okennedy self-requested a review May 1, 2024 00:36
Copy link
Contributor

@okennedy okennedy left a comment

Choose a reason for hiding this comment

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

Main observations

  • Drop the new parameter types, as noted in the comments. I encourage you to factor the rendering code out into info.vizierdb.ui.widgets, but many of them don't need to be new parameter types. (Multiselect is a legitimate new use-case)
  • Delete all the build-specific crap that's getting committed. That doesn't belong in a build tree
  • Fix ChartEditor.loadState

@@ -167,14 +167,6 @@ object Vizier
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is a commit to the plotting tools making changes to Vizier.scala?

@@ -291,6 +323,7 @@ case class Artifact(
datasetDescriptor.properties.get(name)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

spurious extra space?

}

case class ColorParameter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be a brand new parameter type? It seems like this could be a string parameter, just with a new frontend.

Specifically, Parameter types are part of the formal Vizier Frontend/Backend API. Adding a new parameter commits Vizier to supporting it in the long term. This makes extensibility far more difficult and painful. Before adding a new parameter (rather than a new frontend for an existing parameter), we need to ask whether the new parameter is strictly needed.



case class AggregateParameter(
Copy link
Contributor

Choose a reason for hiding this comment

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

AggregateParameter could be an enum parameter

range = Some(VegaRange.Category),
domain = Some(VegaDomain.Literal(series.names.map { JsString(_) }))
domain = Some(VegaDomain.Literal(yAxisLabels.map(JsString(_))))
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed today. Revert this change back to series.


override def currentState: Seq[CommandArgument] =
{
println(datasetRows.now)
Copy link
Contributor

Choose a reason for hiding this comment

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

see note about logger.

@@ -209,10 +209,10 @@ object CommandList
),
"Visualize" -> Seq(
SpecialCommand(label = "Line", icon = "line_plot", packageId = "plot", commandId = "line-chart", description = "Visualize datasets as a a series of lines (with or without points)"),
SpecialCommand(label = "Scatterplot", icon = "scatter_plot", packageId = "plot", commandId = "scatterplot", description = "Visualize datasets as a series of colored points"),
SpecialCommand(label = "Scatterplot2.0", icon = "scatter_plot", packageId = "plot", commandId = "scatterplot", description = "Visualize datasets as a series of colored points"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the 2.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Delete empty file

@@ -62,8 +66,21 @@ class Dataset(
fetchRowsWithAPI,
(candidates, pageSize) => candidates.maxBy { pageIdx => math.abs(pageIdx - table.firstRowIndex) }
)
val datasetSummary = new DatasetSummary(projectId, datasetId)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an Rx. Don't use raw dom manipulation if avoidable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again... don't commit this garbage.

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.

None yet

5 participants