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

ArcInvestigation level assays #184

Merged
merged 17 commits into from
Sep 18, 2023
Merged

ArcInvestigation level assays #184

merged 17 commits into from
Sep 18, 2023

Conversation

Freymaurer
Copy link
Collaborator

  • Add parent option for ArcAssay and ArcStudy
  • Move full ArcAssays out of ArcStudy to ArcInvestigation

@Freymaurer Freymaurer changed the base branch from main to developer September 8, 2023 07:21
@HLWeil HLWeil force-pushed the developer_TopLevelAssays branch 2 times, most recently from b6e8270 to 93aa210 Compare September 12, 2023 14:22
@HLWeil
Copy link
Member

HLWeil commented Sep 17, 2023

CI bugged?

image

Everything works for me locally. I even tried cloning the repo again (which also worked).

@HLWeil
Copy link
Member

HLWeil commented Sep 17, 2023

Also: The erroneous line does not even exist:

image

https://github.com/nfdi4plants/ARCtrl/blob/developer_TopLevelAssays/src/ARCtrl/ARCtrl.fs

@HLWeil
Copy link
Member

HLWeil commented Sep 17, 2023

Any ideas? @Freymaurer @kMutagene

Copy link
Collaborator Author

@Freymaurer Freymaurer left a comment

Choose a reason for hiding this comment

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

Please update naming and visibility issues ❤️

@@ -32,6 +32,14 @@ type ArcTable =
Values = System.Collections.Generic.Dictionary<int*int,CompositeCell>()
}

static member initWithHeaders(name,headers : ResizeArray<CompositeHeader>) =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

createFromHeaders

@@ -156,7 +164,7 @@ type ArcTable =
column.Cells |> Array.iteri (fun rowIndex v -> Unchecked.setCellAt(columnIndex,rowIndex,v) this.Values)
Unchecked.fillMissingCells this.Headers this.Values

static member updatetColumn (columnIndex:int, header: CompositeHeader, ?cells: CompositeCell []) =
static member updatedColumn (columnIndex:int, header: CompositeHeader, ?cells: CompositeCell []) =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please update to make difference to setColumn clear

|> fun (headers, rows) -> ArcTable.create(name,headers,rows)

/// This method is meant to update an ArcTable stored as a protocol in a study or investigation file with the information from an ArcTable actually stored as an annotation table
member this.UpdateReferenceByAnnotationTable(table:ArcTable) =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe move to a less discoverable location

for c in table.Columns do
this.AddColumn(c.Header, cells = c.Cells,forceReplace = true)

static member SplitByColumnValues(columnIndex) =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SplitByColumnValuesAt; and comment, that this divided table rows

ArcTable.createFromRows(table.Name,headers,rows)
)

static member SplitByColumnValuesByHeader(header : CompositeHeader) =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

SplitByColumnValues, and add comment that this divides table by rows

@@ -275,6 +278,16 @@ module Unchecked =
for missingColumn,missingRow in missingKeys do
setCellAt (missingColumn,missingRow,empty) values

/// Increases the table size to the given new row count and fills the new rows with the last value of the column
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe combine with fillMissingCells and add a flag to use empty or last value

Copy link
Member

Choose a reason for hiding this comment

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

Now that I think about it, it doesn't really match. ExtendRows does not fill up empty cells in a previously sparse table, but extends the table to a new given rowCount.

|> ArcTables

static member updateReferenceTablesBySheets (referenceTables : ArcTables,sheetTables : ArcTables,?keepUnusedRefTables : bool) : ArcTables =
let keepUnusedRefTables = Option.defaultValue false keepUnusedRefTables
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we move this somewhere less visible?

Copy link
Member

Choose a reason for hiding this comment

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

Will first make a static member out of it. We need a place for more complex operations that do not necessarily belong to a downstream repository.

Copy link
Member

Choose a reason for hiding this comment

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

Whoops, this comment belongs to your comment above. Still my point remains.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe a module similiar to module ARCtrl.ISA.IdentifierSetters at the end of the arctypes folder with extensions?

@@ -1,5 +1,17 @@
namespace ARCtrl.ISA.Aux

module List =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

make this internal? So we never flood namespaces.

Copy link
Collaborator Author

@Freymaurer Freymaurer left a comment

Choose a reason for hiding this comment

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

👍 lgtm

@HLWeil HLWeil merged commit b18400f into developer Sep 18, 2023
2 checks passed
This was referenced Sep 19, 2023
@HLWeil HLWeil deleted the developer_TopLevelAssays branch September 20, 2023 13:07
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