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.

Sign up for GitHub

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

Jump to bottom

Toggle graphs in the training board olena #825

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Toggle graphs in the training board olena #825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
39a16ed
hide and show advanced info about training
iamsherlocked1891 Oct 15, 2024
b7e0159
deleting training logs
iamsherlocked1891 Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion webapp/src/components/containers/DropdownCard.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,26 @@
</template>

<script setup lang="ts">
import { ref } from "vue";
import { ref, onMounted } from "vue";

import UpArrow from "@/assets/svg/UpArrow.vue";
import DownArrow from "@/assets/svg/DownArrow.vue";

import IconCardHeader from "./IconCardHeader.vue";

const props = defineProps({
initiallyOpen: {
type: String,
default: "true", //open by default
},
});
Comment on lines +34 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally find it clearer to use type-only props declarations. also, you can have a preciser type (only two value are possible here, true or false) and avoid JSON.parse at all.

Suggested change
const props = defineProps({
initiallyOpen: {
type: String,
default: "true", //open by default
},
});
const props = withDefaults(
defineProps<{
initiallyOpen?: boolean;
}>(),
{ initiallyOpen: true },
);


const opened = ref(true);

onMounted(() => {
opened.value = JSON.parse(props.initiallyOpen);
});
Comment on lines +43 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

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

onMounted is quite special, it doesn't get triggered when data change for example. better to watch for prop to change instead, so that if someone change the prop from the outside, it will behave as expected.

Suggested change
onMounted(() => {
opened.value = JSON.parse(props.initiallyOpen);
});
watch(props, ({ initiallyOpen }) => {
opened.value = initiallyOpen;
});


function toggle() {
opened.value = !opened.value;
}
Expand Down
42 changes: 16 additions & 26 deletions webapp/src/components/training/TrainingInformation.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@
</IconCardSmall>
</div>

<!-- Training and validation loss charts -->
<div
<DropdownCard :initiallyOpen="currentOpen" @click="toggleAdvancedInfo">
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider renaming currentOpen to initiallyOpen, then you can use the short version of the attribute, which is a bit nicer

Suggested change
<DropdownCard :initiallyOpen="currentOpen" @click="toggleAdvancedInfo">
<DropdownCard :initiallyOpen @click="toggleAdvancedInfo">

<template #title> Advanced information </template>
<div
class="flex flex-col md:grid gap-4 md:gap-8"
:class="hasValidationData ? 'md:grid-cols-2' : ''"
>
Expand Down Expand Up @@ -146,35 +147,14 @@
/>
</IconCard>
</div>

<IconCard>
<template #title> Training Logs </template>
<template #icon> <Contact /> </template>

<!-- Scrollable training logs -->
<div id="mapHeader" class="max-h-80 overflow-y-auto">
<ul class="grid grid-cols-1">
<li
v-for="(message, index) in props.messages"
:key="index"
class="border-slate-400"
>
<span
style="white-space: pre-line"
class="text-sm text-slate-500 dark:text-slate-400"
>
{{ message }}
</span>
</li>
</ul>
</div>
</IconCard>
</DropdownCard>
<!-- Training and validation loss charts -->
</div>
</template>

<script setup lang="ts">
import { List } from "immutable";
import { computed } from "vue";
import { computed, ref } from "vue";
import ApexChart from "vue3-apexcharts";

import type { BatchLogs, EpochLogs, RoundLogs } from "@epfml/discojs";
Expand All @@ -185,7 +165,10 @@
import ModelExchangeIcon from "@/assets/svg/ModelExchangeIcon.vue";
import ModelUpdateIcon from "@/assets/svg/ModelUpdateIcon.vue";
import PeopleIcon from "@/assets/svg/PeopleIcon.vue";
import Contact from "@/assets/svg/Contact.vue";

Check failure on line 168 in webapp/src/components/training/TrainingInformation.vue

View workflow job for this annotation

GitHub Actions / lint-webapp 

'Contact' is defined but never used. Allowed unused vars must match /^_/u
import DropdownCard from "../containers/DropdownCard.vue";

const currentOpen = ref(localStorage.getItem('initiallyOpen') || 'false');
Copy link
Collaborator

Choose a reason for hiding this comment

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

now that it is a boolean, parsing as to be updated, as close as the loading point you can. it helps with debugging because you know where the value is created. an easy parser is shown below.
also, the "or" op is quite unsafe (as many values can be falsy), better to use a nullish coaslescing which only check for null and undefined.

Suggested change
const currentOpen = ref(localStorage.getItem('initiallyOpen') || 'false');
const currentOpen = ref(localStorage.getItem("initiallyOpen") === "true" ? true : false);

one can even make it safer by checking for the full valid range, which would be the nicest (as it fails on unexpected values, which is always nice for finding logical bugs)

function loadCurrentOpen(): boolean {
  const raw = localStorage.getItem("initiallyOpen");
  switch (raw) {
    case "true":
      return true;
    case "false":
    case null:
      return false;
    default:
      throw new Error(`unexpected loadCurrent stored value: ${raw}`);
  }
}
const currentOpen = ref(loadCurrentOpen());


const props = defineProps<{
rounds: List<RoundLogs>;
Expand Down Expand Up @@ -359,4 +342,11 @@
function percent(n: number): string {
return (n * 100).toFixed(2);
}
// Function to toggle the advanced information
const toggleAdvancedInfo = () => {
const newOpen = currentOpen.value === 'false' ? 'true' : 'false';
localStorage.setItem('initiallyOpen', newOpen);
currentOpen.value = newOpen;
};

</script>
Loading